Re: [Maria-developers] 19aebbd1b89: MDEV-16481: set global system_versioning_asof=sf() crashes in specific case
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(<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 security@mariadb.org
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@gmail.com> AuthorDate: Mon Jul 22 19:12:15 2019 +1000 Commit: Nikita Malyavin <nikitamalyavin@gmail.com> CommitDate: Tue Apr 6 21:32:43 2021 +0300 Regards, Nikita On Tue, 13 Jul 2021 at 06:28, Sergei Golubchik <serg@mariadb.org> 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@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
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 =
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
Hi, Nikita! On Jul 19, Nikita Malyavin wrote:
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 didn't review it, did I? All I have is 2d73406dad8: MDEV-16481: set global system_versioning_asof=sf() crashes in specific case 19aebbd1b89: MDEV-16481: set global system_versioning_asof=sf() crashes in specific case
[ I think we really should consent some more automatizing tooling to better track the discussions]
Isn't it enough to just use one tool consistently? It seems that the confusion here comes from us discussing MDEV-16481 somewhere else, not in the email. No automatic tooling would help if we bypass it and discuss somewhere else.
The commits can be found at bb-10.3-nikita-old for now.
I'll take a look
2. The email in your header is wrong, I never used it in git, and besides format=fuller shows the correct one:
Right, that's my mailmap :) It only affects where review emails are sent. Also once I was interested to count internal vs external commits, and this mailmap helped to attribute them correctly. If that's confusing I think I can use mailmapped author only as the email recipient, but inclide not-mailmapped commit into the email body.
commit 19aebbd1b89feb1482e2cdf5ddb8322f48ad4216 Author: Nikita Malyavin <nikitamalyavin@gmail.com> AuthorDate: Mon Jul 22 19:12:15 2019 +1000 Commit: Nikita Malyavin <nikitamalyavin@gmail.com> CommitDate: Tue Apr 6 21:32:43 2021 +0300
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
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Nikita! On Jul 19, Sergei Golubchik wrote:
The commits can be found at bb-10.3-nikita-old for now.
I'll take a look
The diff 19aebbd1b89fe^..01ab3db8c73 looks pretty good to me, ok to push Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Nikita Malyavin -
Sergei Golubchik