maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Hello, Sergei!

1. What you ask was done in 01ab3db8c7 "make all conversions in check() to
avoid possible errors". It was mentioned in the previous discussion btw

[ I think we really should consent some more automatizing tooling to better
track the discussions]

The commits can be found at bb-10.3-nikita-old for now.

2. The email in your header is wrong, I never used it in git, and besides
format=fuller shows the correct one:

commit 19aebbd1b89feb1482e2cdf5ddb8322f48ad4216
Author:     Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
AuthorDate: Mon Jul 22 19:12:15 2019 +1000
Commit:     Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
CommitDate: Tue Apr 6 21:32:43 2021 +0300

Regards,
Nikita

On Tue, 13 Jul 2021 at 06:28, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> 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@xxxxxxxxxxx>
> > committer: Nikita Malyavin <nikita.malyavin@xxxxxxxxxxx>
> > 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@xxxxxxxxxxx
>
> _______________________________________________
> Mailing list: https://launchpad.net/~maria-developers
> Post to     : maria-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~maria-developers
> More help   : https://help.launchpad.net/ListHelp
>


-- 
Yours truly,
Nikita Malyavin