maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxx>
> committer: Sergei Golubchik <serg@xxxxxxxxxxx>
> 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(&ltime, 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, &ltime, 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, &ltime, opt);
> +        out.unix_time= pool.time_zone->TIME_to_gmt_sec(&ltime, &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(&ltime, val.unix_time);
> +      ltime.second_part= val.second_part;
> +
> +      if (buf && !my_datetime_to_str(&ltime, (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@xxxxxxxxxxx