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 AuthorDate: Mon Jul 22 19:12:15 2019 +1000 Commit: Nikita Malyavin CommitDate: Tue Apr 6 21:32:43 2021 +0300 Regards, Nikita On Tue, 13 Jul 2021 at 06:28, Sergei Golubchik wrote:

Hi, Nikita!

On Jul 12, Nikita Malyavin wrote:

...

revision-id: 19aebbd1b89 (mariadb-10.3.26-129-g19aebbd1b89) parent(s): 10c163e4a18 author: Nikita Malyavin committer: Nikita Malyavin 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

...

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(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 =

keycache_name.variable_name */ 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 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

-- Yours truly, Nikita Malyavin