Hi, Nikita!

On Jul 12, Nikita Malyavin wrote:
> revision-id: 19aebbd1b89 (mariadb-10.3.26-129-g19aebbd1b89)
> parent(s): 10c163e4a18
> author: Nikita Malyavin <nikita.malyavin@mariadb.com>
> committer: Nikita Malyavin <nikita.malyavin@mariadb.com>
> timestamp: 2021-04-06 21:32:43 +0300
> message:
>
> MDEV-16481: set global system_versioning_asof=sf() crashes in specific case
>
> * sys_vars.h: add `MYSQL_TIME` field to `set_var::save_result`
> * sys_vars.ic: get rid of calling `var->value->get_date()` from `Sys_var_vers_asof::update()`
> * versioning.sysvars: add test; remove double warning
>
> diff --git a/sql/set_var.h b/sql/set_var.h
> index 12e025e4696..6351fb5b666 100644
> --- a/sql/set_var.h
> +++ b/sql/set_var.h
> @@ -296,6 +296,7 @@ class set_var :public set_var_base
>      plugin_ref *plugins;                ///< for Sys_var_pluginlist
>      Time_zone *time_zone;               ///< for Sys_var_tz
>      LEX_STRING string_value;            ///< for Sys_var_charptr and others
> +    MYSQL_TIME time;                    ///< for Sys_var_vers_asof

old sizeof(save_result) is 16 bytes, sizeof(MYSQL_TIME) is 40.

It's just an observation, set_var isn't a structure that's constantly in
memory in large quantities. So, not a problem at all.

>      const void *ptr;                    ///< for Sys_var_struct
>    } save_result;
>    LEX_CSTRING base; /**< for structured variables, like keycache_name.variable_name */
> diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic
> index 8d094d3a1d1..eff6a59e65b 100644
> --- a/sql/sys_vars.ic
> +++ b/sql/sys_vars.ic
> @@ -2650,50 +2650,47 @@ class Sys_var_vers_asof: public Sys_var_enum
>    virtual bool do_check(THD *thd, set_var *var)
>    {
>      if (!Sys_var_enum::do_check(thd, var))
> +    {
> +      var->save_result.time.time_type= MYSQL_TIMESTAMP_NONE;
>        return false;
> -    MYSQL_TIME ltime;
> -    bool res= var->value->get_date(&ltime, TIME_NO_ZERO_IN_DATE|TIME_NO_ZERO_DATE);
> -    if (!res)
> +    }
> +    bool res= var->value->get_date(&var->save_result.time,
> +                                   TIME_NO_ZERO_IN_DATE | TIME_NO_ZERO_DATE);
> +    if (res)
>      {
> -      var->save_result.ulonglong_value= SYSTEM_TIME_AS_OF;
> +      var->save_result.time.time_type= MYSQL_TIMESTAMP_ERROR;
>      }
>      return res;
>    }
> 
>  private:
> -  bool update(set_var *var, vers_asof_timestamp_t &out, system_variables& pool)
> +  bool update(THD *thd, set_var *var, vers_asof_timestamp_t *out)
>    {
> -    bool res= false;
> -    uint error;
> -    MYSQL_TIME ltime;
> -    out.type= static_cast<enum_var_type>(var->save_result.ulonglong_value);
> -    if (out.type == SYSTEM_TIME_AS_OF)
> +    MYSQL_TIME &ltime= var->save_result.time;
> +    uint error= 0;
> +
> +    if (var->value && ltime.time_type >= 0) // any valid value is ok
>      {
> -      if (var->value)
> -      {
> -        res= var->value->get_date(&ltime, TIME_NO_ZERO_IN_DATE
> -                                          | TIME_NO_ZERO_DATE);
> -        out.unix_time= pool.time_zone->TIME_to_gmt_sec(&ltime, &error);
> -        out.second_part= ltime.second_part;
> -        res|= (error != 0);
> -      }
> -      else // set DEFAULT from global var
> -      {
> -        out.type= SYSTEM_TIME_UNSPECIFIED;
> -        res= false;
> -      }
> +      out->type       = SYSTEM_TIME_AS_OF;
> +      out->unix_time  = thd->variables.time_zone->TIME_to_gmt_sec(&ltime,
> +                                                                  &error);

why are you doing it here? You could've converted in ::check(),
and as a bonus you wouldn't need MYSQL_TIME in the save_result.
And what would you do if TIME_to_gmt_sec would fail? ::update() isn't
expected to fail, ::check() should guarantee that as much as possible.

> +      out->second_part= ltime.second_part;
>      }
> -    return res;
> +    else // DEFAULT is set
> +    {
> +      out->type= SYSTEM_TIME_UNSPECIFIED;
> +    }
> +    return (error != 0);
>    }
> 
>  public:
>    virtual bool global_update(THD *thd, set_var *var)
>    {
> -    return update(var, global_var(vers_asof_timestamp_t), thd->variables);
> +    return update(thd, var, &global_var(vers_asof_timestamp_t));
>    }
>    virtual bool session_update(THD *thd, set_var *var)
>    {
> -    return update(var, session_var(thd, vers_asof_timestamp_t), thd->variables);
> +    return update(thd, var, &session_var(thd, vers_asof_timestamp_t));
>    }
> 
>  private:
>
Regards,
Sergei
VP of MariaDB Server Engineering
and security@mariadb.org

_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to     : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp