maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx