Re: [Maria-developers] 19aebbd1b89: MDEV-16481: set global system_versioning_asof=sf() crashes in specific case

18 Jul 2021

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 
AuthorDate: Mon Jul 22 19:12:15 2019 +1000
Commit:     Nikita Malyavin 
CommitDate: Tue Apr 6 21:32:43 2021 +0300

Regards,
Nikita

On Tue, 13 Jul 2021 at 06:28, Sergei Golubchik  wrote:
...
Hi, Nikita!
On Jul 12, Nikita Malyavin wrote:
...
revision-id: 19aebbd1b89 (mariadb-10.3.26-129-g19aebbd1b89)
parent(s): 10c163e4a18
author: Nikita Malyavin 
committer: Nikita Malyavin 
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(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

    