Re: [Maria-developers] f3fc58149dd: MDEV-29159 Patch for MDEV-28918 introduces more inconsistency than it solves, breaks usability
Hi, Alexander, A couple of comments: On Aug 03, Alexander Barkov wrote:
revision-id: f3fc58149dd (mariadb-10.7.4-39-gf3fc58149dd) parent(s): 97d16c7544c author: Alexander Barkov committer: Alexander Barkov timestamp: 2022-08-03 11:28:31 +0400 message:
MDEV-29159 Patch for MDEV-28918 introduces more inconsistency than it solves, breaks usability
Store assignment failures on incompatible data types now cause errors if: - STRICT_ALL_TABLES or STRICT_TRANS_TABLES sql_mode is used, and - IGNORE is not used
Otherwise, only a warning is raised and the statement continues.
TODO: add MTR tests for "loose" modes.
diff --git a/sql/field.cc b/sql/field.cc index 249269a6b1d..e997e2cbd69 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -964,7 +964,7 @@ Type_handler::aggregate_for_result_traditional(const Type_handler *a, }
-bool Field::check_assignability_from(const Type_handler *from) const +bool Field::check_assignability_from(const Type_handler *from, bool error) const
"error" is very confusing here, I had to read the whole patch to understand what it means. Better names: * only_warning * ignore * strict * issue_error
{ /* Using type_handler_for_item_field() here to get the data type handler @@ -982,6 +982,15 @@ bool Field::check_assignability_from(const Type_handler *from) const type_handler_for_item_field()); if (th.aggregate_for_result(from->type_handler_for_item_field())) { + if (!error) + { + THD *thd= get_thd(); + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, + ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, + ER_THD(thd, ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION), + type_handler()->name().ptr(), from->name().ptr(), "SET"); + return false; + } my_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, MYF(0), type_handler()->name().ptr(), from->name().ptr(), "SET"); return true;
if you wouldn't want to change the error message in a followup commit, I'd suggested something like my_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, MYF(error ? 0 : ME_WARNING), type_handler()->name().ptr(), from->name().ptr(), "SET"); return error; Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hello Sergei, Please have a look into a modified patch: https://github.com/MariaDB/server/commit/a507f126b03bfa18fdc7a1d87f368231bb7... Changes comparing to the previous version: - Review suggestions addressed - Added thorough tests for sql_mode='' and for strict+IGNORE. - Changed the error message, so the user can see the column (or the SP variable) name which causes the problem. On 8/4/22 1:00 AM, Sergei Golubchik wrote:
Hi, Alexander,
A couple of comments:
<cut>
-bool Field::check_assignability_from(const Type_handler *from) const +bool Field::check_assignability_from(const Type_handler *from, bool error) const
"error" is very confusing here, I had to read the whole patch to understand what it means. Better names: * only_warning * ignore * strict * issue_error
Thanks. I chose "ignore". It not only makes the purpose of the parameter clearer, but also helps to avoid code duplication: the condition mixing IGNORE and sql_mode flags is now tested only in one place: when raising an error or a warning.
{ /* Using type_handler_for_item_field() here to get the data type handler @@ -982,6 +982,15 @@ bool Field::check_assignability_from(const Type_handler *from) const type_handler_for_item_field()); if (th.aggregate_for_result(from->type_handler_for_item_field())) { + if (!error) + { + THD *thd= get_thd(); + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, + ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, + ER_THD(thd, ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION), + type_handler()->name().ptr(), from->name().ptr(), "SET"); + return false; + } my_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, MYF(0), type_handler()->name().ptr(), from->name().ptr(), "SET"); return true;
if you wouldn't want to change the error message in a followup commit, I'd suggested something like
my_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, MYF(error ? 0 : ME_WARNING), type_handler()->name().ptr(), from->name().ptr(), "SET"); return error;
Thanks for the idea. I did not know this trick. And it also works with my_printf_error: + bool error= !ignore && get_thd()->is_strict_mode(); + my_printf_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, + "Unknown CAST(%s AS %s) in assignment of '%s'", + MYF(error ? 0 : ME_WARNING), + from->name().ptr(), type_handler()->name().ptr(), + field_name.str);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik