Hi, Nikita! Okay. This reverts my "ok to push" on MDEV-16481 :) If you want to store the value in unix timestamp, you don't need MYSQL_TIME in save_result. You should calculate the value as a unix timestamp in ::check and store that in save_result. Now you still have timezone conversion in ::update and that can fail. Everything that can fail should be done in ::check, that's the contract. May be also I reapplied your patches to 10.3 incorrectly when I was reviewing. Perhaps you could rebase them yourself - to be sure I'm looking at the correct patch? On Oct 22, Nikita Malyavin wrote:
revision-id: 05968211699 (mariadb-10.4.11-291-g05968211699) parent(s): efb1023b6f4 author: Nikita Malyavin <nikitamalyavin@gmail.com> committer: Sergei Golubchik <serg@mariadb.com> timestamp: 2020-10-22 17:07:03 +0200 message:
MDEV-16026: Global system_versioning_asof must not be used if client sessions can have non-default time zone
* store `system_versioning_asof` in unix time; * both session and global vars are processed in session timezone; * setting `default` does not copy global varibale abymore. Instead, it sets system_time to SYSTEM_TIME_UNSPECIFIED, which means that no 'AS OF' time is applied and `now()` can be assumed As a regression, we cannot assign values below 1970 anymore
--- mysql-test/suite/versioning/r/sysvars.result | 67 +++++++++++++++++++++++ mysql-test/suite/versioning/t/sysvars.test | 80 ++++++++++++++++++++++++---- sql/mysqld.h | 3 +- sql/sql_select.cc | 6 ++- sql/sys_vars.ic | 27 ++++++---- 5 files changed, 162 insertions(+), 21 deletions(-)
diff --git a/sql/mysqld.h b/sql/mysqld.h index bd45ff7b798..884e5a06066 100644 --- a/sql/mysqld.h +++ b/sql/mysqld.h @@ -194,7 +194,8 @@ enum vers_system_time_t struct vers_asof_timestamp_t { ulong type; - MYSQL_TIME ltime; + my_time_t unix_time; + ulong second_part; };
enum vers_alter_history_enum diff --git a/sql/sql_select.cc b/sql/sql_select.cc index d0bb0c816ec..4d62a9829eb 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -722,8 +722,12 @@ bool vers_select_conds_t::init_from_sysvar(THD *thd) if (type != SYSTEM_TIME_UNSPECIFIED && type != SYSTEM_TIME_ALL) { DBUG_ASSERT(type == SYSTEM_TIME_AS_OF); + MYSQL_TIME ltime; + thd->variables.time_zone->gmt_sec_to_TIME(<ime, in.unix_time); + ltime.second_part = in.second_part; + start.item= new (thd->mem_root) - Item_datetime_literal(thd, &in.ltime, TIME_SECOND_PART_DIGITS); + Item_datetime_literal(thd, <ime, TIME_SECOND_PART_DIGITS); if (!start.item) return true; } diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index f33f469b160..417fa7842c2 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -2646,9 +2646,11 @@ class Sys_var_vers_asof: public Sys_var_enum }
private: - bool update(set_var *var, vers_asof_timestamp_t &out) + bool update(set_var *var, vers_asof_timestamp_t &out, system_variables& pool) { 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) { @@ -2658,11 +2660,14 @@ class Sys_var_vers_asof: public Sys_var_enum Datetime::Options opt(TIME_CONV_NONE | TIME_NO_ZERO_IN_DATE | TIME_NO_ZERO_DATE, thd); - res= var->value->get_date(thd, &out.ltime, opt); + res= var->value->get_date(thd, <ime, opt); + 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= global_var(vers_asof_timestamp_t); + out.type= SYSTEM_TIME_UNSPECIFIED; res= false; } } @@ -2672,11 +2677,11 @@ class Sys_var_vers_asof: public Sys_var_enum public: virtual bool global_update(THD *thd, set_var *var) { - return update(var, global_var(vers_asof_timestamp_t)); + return update(var, global_var(vers_asof_timestamp_t), thd->variables); } virtual bool session_update(THD *thd, set_var *var) { - return update(var, session_var(thd, vers_asof_timestamp_t)); + return update(var, session_var(thd, vers_asof_timestamp_t), thd->variables); }
private: @@ -2690,10 +2695,14 @@ class Sys_var_vers_asof: public Sys_var_enum case SYSTEM_TIME_AS_OF: { uchar *buf= (uchar*) thd->alloc(MAX_DATE_STRING_REP_LENGTH); - if (buf &&!my_datetime_to_str(&val.ltime, (char*) buf, 6)) + MYSQL_TIME ltime; + + thd->variables.time_zone->gmt_sec_to_TIME(<ime, val.unix_time); + ltime.second_part= val.second_part; + + if (buf && !my_datetime_to_str(<ime, (char*) buf, 6)) { - // TODO: figure out variable name - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "system_versioning_asof_timestamp", "NULL (wrong datetime)"); + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name.str, "NULL (wrong datetime)"); return (uchar*) thd->strdup("Error: wrong datetime"); } return buf; @@ -2701,7 +2710,7 @@ class Sys_var_vers_asof: public Sys_var_enum default: break; } - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "system_versioning_asof_timestamp", "NULL (wrong range type)"); + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name.str, "NULL (wrong range type)"); return (uchar*) thd->strdup("Error: wrong range type"); }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org