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 <[email protected]> AuthorDate: Mon Jul 22 19:12:15 2019 +1000 Commit: Nikita Malyavin <[email protected]> CommitDate: Tue Apr 6 21:32:43 2021 +0300 Regards, Nikita On Tue, 13 Jul 2021 at 06:28, Sergei Golubchik <[email protected]> wrote: > Hi, Nikita! > > On Jul 12, Nikita Malyavin wrote: > > revision-id: 19aebbd1b89 (mariadb-10.3.26-129-g19aebbd1b89) > > parent(s): 10c163e4a18 > > author: Nikita Malyavin <[email protected]> > > committer: Nikita Malyavin <[email protected]> > > 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(<ime, > 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 <ime= 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(<ime, TIME_NO_ZERO_IN_DATE > > - | TIME_NO_ZERO_DATE); > > - out.unix_time= pool.time_zone->TIME_to_gmt_sec(<ime, &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(<ime, > > + > &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 [email protected] > > _______________________________________________ > Mailing list: https://launchpad.net/~maria-developers > Post to : [email protected] > Unsubscribe : https://launchpad.net/~maria-developers > More help : https://help.launchpad.net/ListHelp > -- Yours truly, Nikita Malyavin
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
