MDEV-12179: Per-engine mysql.gtid_slave_pos table

Intermediate commit.

Fix engine list lifetime for sys_var_pluginlist.

The Sys_var class assumes that some operations can be done without
explicitly freeing resources, for example default_value_ptr(). Thus,
methods (like Sys_var_pluginlist::do_check) need to generally work
with temporary lists, which are registered in the THD to be
freed/unlocked automatically. And do_update() needs to make a
permanent copy to store in the global variable.

5 files changed, 71 insertions, 25 deletions

diff --git a/sql/mysqld.cc b/sql/mysqld.cc
index 09c9c5c681e..0b8aaf1de78 100644
--- a/sql/mysqld.cc
+++ b/sql/mysqld.cc
@@ -4932,10 +4932,10 @@ init_gtid_pos_auto_engines(void)
     then it will certainly not be needed.
   */
   if (gtid_pos_auto_engines)
-    plugins= resolve_engine_list(gtid_pos_auto_engines,
-                                 strlen(gtid_pos_auto_engines), false);
+    plugins= resolve_engine_list(NULL, gtid_pos_auto_engines,
+                                 strlen(gtid_pos_auto_engines), false, false);
   else
-    plugins= resolve_engine_list("", 0, false);
+    plugins= resolve_engine_list(NULL, "", 0, false, false);
   if (!plugins)
     return 1;
   mysql_mutex_lock(&LOCK_global_system_variables);
diff --git a/sql/set_var.cc b/sql/set_var.cc
index 78904f75661..acc744808cb 100644
--- a/sql/set_var.cc
+++ b/sql/set_var.cc
@@ -1301,17 +1301,18 @@ engine_list_next_item(const char **pos, const char *end_pos,
 
 
 static bool
-resolve_engine_list_item(plugin_ref *list, uint32 *idx,
+resolve_engine_list_item(THD *thd, plugin_ref *list, uint32 *idx,
                          const char *pos, const char *pos_end,
-                         bool error_on_unknown_engine)
+                         bool error_on_unknown_engine, bool temp_copy)
 {
   LEX_STRING item_str;
   plugin_ref ref;
   uint32_t i;
+  THD *thd_or_null = (temp_copy ? thd : NULL);
 
   item_str.str= const_cast<char*>(pos);
   item_str.length= pos_end-pos;
-  ref= ha_resolve_by_name(NULL, &item_str, false);
+  ref= ha_resolve_by_name(thd_or_null, &item_str, false);
   if (!ref)
   {
     if (error_on_unknown_engine)
@@ -1327,7 +1328,8 @@ resolve_engine_list_item(plugin_ref *list, uint32 *idx,
   {
     if (plugin_hton(list[i]) == plugin_hton(ref))
     {
-      plugin_unlock(NULL, ref);
+      if (!temp_copy)
+        plugin_unlock(NULL, ref);
       return false;
     }
   }
@@ -1341,10 +1343,16 @@ resolve_engine_list_item(plugin_ref *list, uint32 *idx,
   Helper for class Sys_var_pluginlist.
   Resolve a comma-separated list of storage engine names to a null-terminated
   array of plugin_ref.
+
+  If TEMP_COPY is true, a THD must be given as well. In this case, the
+  allocated memory and locked plugins are registered in the THD and will
+  be freed / unlocked automatically. If TEMP_COPY is true, THD can be
+  passed as NULL, and resources must be freed explicitly later with
+  free_engine_list().
 */
 plugin_ref *
-resolve_engine_list(const char *str_arg, size_t str_arg_len,
-                    bool error_on_unknown_engine)
+resolve_engine_list(THD *thd, const char *str_arg, size_t str_arg_len,
+                    bool error_on_unknown_engine, bool temp_copy)
 {
   uint32 count, idx;
   const char *pos, *item_start, *item_end;
@@ -1360,7 +1368,10 @@ resolve_engine_list(const char *str_arg, size_t str_arg_len,
     ++count;
   }
 
-  res= (plugin_ref *)my_malloc((count+1)*sizeof(*res), MYF(MY_ZEROFILL|MY_WME));
+  if (temp_copy)
+    res= (plugin_ref *)thd->calloc((count+1)*sizeof(*res));
+  else
+    res= (plugin_ref *)my_malloc((count+1)*sizeof(*res), MYF(MY_ZEROFILL|MY_WME));
   if (!res)
   {
     my_error(ER_OUTOFMEMORY, MYF(0), (int)((count+1)*sizeof(*res)));
@@ -1376,15 +1387,16 @@ resolve_engine_list(const char *str_arg, size_t str_arg_len,
     DBUG_ASSERT(idx < count);
     if (idx >= count)
       break;
-    if (resolve_engine_list_item(res, &idx, item_start, item_end,
-                                 error_on_unknown_engine))
+    if (resolve_engine_list_item(thd, res, &idx, item_start, item_end,
+                                 error_on_unknown_engine, temp_copy))
       goto err;
   }
 
   return res;
 
 err:
-  free_engine_list(res);
+  if (!temp_copy)
+    free_engine_list(res);
   return NULL;
 }
 
@@ -1418,6 +1430,32 @@ copy_engine_list(plugin_ref *list)
   }
   for (i= 0; i < count; ++i)
     p[i]= my_plugin_lock(NULL, list[i]);
+  p[i] = NULL;
+  return p;
+}
+
+
+/*
+  Create a temporary copy of an engine list. The memory will be freed
+  (and the plugins unlocked) automatically, on the passed THD.
+*/
+plugin_ref *
+temp_copy_engine_list(THD *thd, plugin_ref *list)
+{
+  plugin_ref *p;
+  uint32 count, i;
+
+  for (p= list, count= 0; *p; ++p, ++count)
+    ;
+  p= (plugin_ref *)thd->alloc((count+1)*sizeof(*p));
+  if (!p)
+  {
+    my_error(ER_OUTOFMEMORY, MYF(0), (int)((count+1)*sizeof(*p)));
+    return NULL;
+  }
+  for (i= 0; i < count; ++i)
+    p[i]= my_plugin_lock(thd, list[i]);
+  p[i] = NULL;
   return p;
 }
 
diff --git a/sql/set_var.h b/sql/set_var.h
index cdd0f7da1ba..66a3eeae42d 100644
--- a/sql/set_var.h
+++ b/sql/set_var.h
@@ -423,10 +423,11 @@ int sys_var_init();
 uint sys_var_elements();
 int sys_var_add_options(DYNAMIC_ARRAY *long_options, int parse_flags);
 void sys_var_end(void);
-plugin_ref *resolve_engine_list(const char *str_arg, size_t str_arg_len,
-                                bool error_on_unknown_engine);
+plugin_ref *resolve_engine_list(THD *thd, const char *str_arg, size_t str_arg_len,
+                                bool error_on_unknown_engine, bool temp_copy);
 void free_engine_list(plugin_ref *list);
 plugin_ref *copy_engine_list(plugin_ref *list);
+plugin_ref *temp_copy_engine_list(THD *thd, plugin_ref *list);
 char *pretty_print_engine_list(THD *thd, plugin_ref *list);
 
 #endif
diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
index 8f0a081acc7..84b88f84bbf 100644
--- a/sql/sys_vars.cc
+++ b/sql/sys_vars.cc
@@ -3541,11 +3541,6 @@ check_gtid_pos_auto_engines(sys_var *self, THD *thd, set_var *var)
     if (running)
       err= true;
   }
-  if (err && var->save_result.plugins)
-  {
-    free_engine_list(var->save_result.plugins);
-    var->save_result.plugins= NULL;
-  }
   return err;
 }
 
diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic
index c5fe8c0af2c..5227384f203 100644
--- a/sql/sys_vars.ic
+++ b/sql/sys_vars.ic
@@ -1546,6 +1546,17 @@ public:
 
   These variables don't support command-line equivalents, any such
   command-line options should be added manually to my_long_options in mysqld.cc
+
+  Note on lifetimes of resources allocated: We allocate a zero-terminated array
+  of plugin_ref*, and lock the contained plugins. The list in the global
+  variable must be freed (with free_engine_list()). However, the way Sys_var
+  works, there is no place to explicitly free other lists, like the one
+  returned from get_default().
+
+  Therefore, the code needs to work with temporary lists, which are
+  registered in the THD to be automatically freed (and plugins similarly
+  automatically unlocked). This is why do_check() allocates a temporary
+  list, from which do_update() then makes a permanent copy.
 */
 class Sys_var_pluginlist: public sys_var
 {
@@ -1575,9 +1586,9 @@ public:
     plugin_ref *plugins;
 
     if (!(res=var->value->val_str(&str)))
-      plugins= resolve_engine_list("", 0, true);
+      plugins= resolve_engine_list(thd, "", 0, true, true);
     else
-      plugins= resolve_engine_list(res->ptr(), res->length(), true);
+      plugins= resolve_engine_list(thd, res->ptr(), res->length(), true, true);
     if (!plugins)
       return true;
     var->save_result.plugins= plugins;
@@ -1586,7 +1597,7 @@ public:
   void do_update(plugin_ref **valptr, plugin_ref* newval)
   {
     plugin_ref *oldval= *valptr;
-    *valptr= newval;
+    *valptr= copy_engine_list(newval);
     free_engine_list(oldval);
   }
   bool session_update(THD *thd, set_var *var)
@@ -1604,14 +1615,15 @@ public:
   void session_save_default(THD *thd, set_var *var)
   {
     plugin_ref* plugins= global_var(plugin_ref *);
-    var->save_result.plugins= plugins ? copy_engine_list(plugins) : 0;
+    var->save_result.plugins= plugins ? temp_copy_engine_list(thd, plugins) : 0;
   }
   plugin_ref *get_default(THD *thd)
   {
     char *default_value= *reinterpret_cast<char**>(option.def_value);
     if (!default_value)
       return 0;
-    return resolve_engine_list(default_value, strlen(default_value), false);
+    return resolve_engine_list(thd, default_value, strlen(default_value),
+                               false, true);
   }
 
   void global_save_default(THD *thd, set_var *var)