maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexey!

On Mar 13, Alexey Botchkov wrote:
> revision-id: 398ea6ab012938d08126c8d0e0291000d06f6cd6 (mariadb-10.2.13-43-g398ea6a)
> parent(s): 9d95b8665a3d87ab857e77e220effc64454eb881
> committer: Alexey Botchkov
> timestamp: 2018-03-13 17:13:27 +0400
> message:
> 
> MDEV-11917 enum/set command-line options aren't respecting max-* settings.
> 
>         Code added to make maximum- working for Sys_var_enum
>         and Sys_var_set.
> 
> diff --git a/sql/set_var.cc b/sql/set_var.cc
> index e96e636..32e52a9 100644
> --- a/sql/set_var.cc
> +++ b/sql/set_var.cc
> @@ -452,6 +452,22 @@ void sys_var::do_deprecated_warning(THD *thd)
>  
>    @retval         true on error, false otherwise (warning or ok)
>   */
> +
> +
> +bool throw_bounds_warning(THD *thd, const char *name,const char *v)
> +{
> +  if (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES)
> +  {
> +    my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name, v);

This is rather illogical, setting a variable doesn't touch any tables,
so MODE_STRICT_ALL_TABLES should not affect it.

Also, the error message could be improved.

But it's how it was, and it's not subject of this bugfix, I agree.

> +    return true;
> +  }
> +  push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
> +                      ER_TRUNCATED_WRONG_VALUE,
> +                      ER_THD(thd, ER_TRUNCATED_WRONG_VALUE), name, v);
> +  return false;
> +}
> +
> +
>  bool throw_bounds_warning(THD *thd, const char *name,
>                            bool fixed, bool is_unsigned, longlong v)
>  {
> @@ -464,14 +480,7 @@ bool throw_bounds_warning(THD *thd, const char *name,
>      else
>        llstr(v, buf);
>  
> -    if (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES)
> -    {
> -      my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name, buf);
> -      return true;
> -    }
> -    push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
> -                        ER_TRUNCATED_WRONG_VALUE,
> -                        ER_THD(thd, ER_TRUNCATED_WRONG_VALUE), name, buf);

ah, good. only one place to change when we fix the above.
thanks.

> +    return throw_bounds_warning(thd, name, buf);
>    }
>    return false;
>  }
> diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic
> index bc913b1..adcda5d 100644
> --- a/sql/sys_vars.ic
> +++ b/sql/sys_vars.ic

all ok, looks good. Just one comment. Please make max_var_ptr() logic to
be in one place only. If C++ cannot do it, then, in the worst case, make
it a macro, like

#define define_max_var_ptr(TYPE)  \
  TYPE *max_var_ptr() { return scope() == SESSION ? (TYPE*)(((uchar*)&max_system_variables) + offset) : 0; }

after that ok to push, thanks!

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx