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