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 <nikitamalya...@gmail.com>
> committer: Sergei Golubchik <s...@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 secur...@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
