plugin can signal that it cannot be unloaded by failing deinit()

if plugin->deinit() returns a failure, it is no longer ignored, it
means that the plugin isn't ready to be unloaded from memory yet.
So it's marked "dying", deinitialized as much as possible,
but stays in memory until shutdown.

also:

* increment MARIA_PLUGIN_INTERFACE_VERSION
* rewrite ha_rocksdb to use the new approach, update the test

sql/encryption.cc

@@ -109,6 +109,7 @@ int initialize_encryption_plugin(st_plugin_int *plugin)
 
 int finalize_encryption_plugin(st_plugin_int *plugin)
 {
+  int deinit_status= 0;
   bool used= plugin_ref_to_int(encryption_manager) == plugin;
 
   if (used)
@@ -118,18 +119,15 @@ int finalize_encryption_plugin(st_plugin_int *plugin)
     encryption_handler.encryption_ctx_size_func= zero_size;
   }
 
-  if (plugin && plugin->plugin->deinit && plugin->plugin->deinit(NULL))
-  {
-    DBUG_PRINT("warning", ("Plugin '%s' deinit function returned error.",
-                           plugin->name.str));
-  }
+  if (plugin && plugin->plugin->deinit)
+    deinit_status= plugin->plugin->deinit(NULL);
 
   if (used)
   {
     plugin_unlock(NULL, encryption_manager);
     encryption_manager= 0;
   }
-  return 0;
+  return deinit_status;
 }
 
 /******************************************************************

sql/handler.cc

@@ -581,6 +581,7 @@ static int hton_drop_table(handlerton *hton, const char *path)
 
 int ha_finalize_handlerton(st_plugin_int *plugin)
 {
+  int deinit_status= 0;
   handlerton *hton= (handlerton *)plugin->data;
   DBUG_ENTER("ha_finalize_handlerton");
 
@@ -595,18 +596,7 @@ int ha_finalize_handlerton(st_plugin_int *plugin)
     hton->panic(hton, HA_PANIC_CLOSE);
 
   if (plugin->plugin->deinit)
-  {
-    /*
-      Today we have no defined/special behavior for uninstalling
-      engine plugins.
-    */
-    DBUG_PRINT("info", ("Deinitializing plugin: '%s'", plugin->name.str));
-    if (plugin->plugin->deinit(NULL))
-    {
-      DBUG_PRINT("warning", ("Plugin '%s' deinit function returned error.",
-                             plugin->name.str));
-    }
-  }
+    deinit_status= plugin->plugin->deinit(NULL);
 
   free_sysvar_table_options(hton);
   update_discovery_counters(hton, -1);
@@ -627,7 +617,7 @@ int ha_finalize_handlerton(st_plugin_int *plugin)
   my_free(hton);
 
  end:
-  DBUG_RETURN(0);
+  DBUG_RETURN(deinit_status);
 }
 
 

sql/sql_audit.cc

@@ -350,14 +350,11 @@ static my_bool calc_class_mask(THD *thd, plugin_ref plugin, void *arg)
 */
 int finalize_audit_plugin(st_plugin_int *plugin)
 {
+  int deinit_status= 0;
   unsigned long event_class_mask[MYSQL_AUDIT_CLASS_MASK_SIZE];
 
-  if (plugin->plugin->deinit && plugin->plugin->deinit(NULL))
-  {
-    DBUG_PRINT("warning", ("Plugin '%s' deinit function returned error.",
-                            plugin->name.str));
-    DBUG_EXECUTE("finalize_audit_plugin", return 1; );
-  }
+  if (plugin->plugin->deinit)
+    deinit_status= plugin->plugin->deinit(NULL);
 
   plugin->data= NULL;
   bzero(&event_class_mask, sizeof(event_class_mask));
@@ -376,7 +373,7 @@ int finalize_audit_plugin(st_plugin_int *plugin)
   bmove(mysql_global_audit_mask, event_class_mask, sizeof(event_class_mask));
   mysql_mutex_unlock(&LOCK_audit_mask);
 
-  return 0;
+  return deinit_status;
 }
 
 

sql/sql_plugin.cc

@@ -1267,24 +1267,22 @@ static void plugin_deinitialize(struct st_plugin_int *plugin, bool ref_check)
   if (!deinit)
     deinit= (plugin_type_init)(plugin->plugin->deinit);
 
-  if (deinit && deinit(plugin))
-  {
-    sql_print_error("Plugin '%s' of type %s failed deinitialization",
-                    plugin->name.str, plugin_type_names[plugin->plugin->type].str);
-  }
-
-  plugin->state= PLUGIN_IS_UNINITIALIZED;
+  if (!deinit || !deinit(plugin))
+    plugin->state= PLUGIN_IS_UNINITIALIZED; // free to unload
 
   if (ref_check && plugin->ref_count)
     sql_print_error("Plugin '%s' has ref_count=%d after deinitialization.",
                     plugin->name.str, plugin->ref_count);
   plugin_variables_deinit(plugin);
 }
 
-static void plugin_del(struct st_plugin_int *plugin)
+static void plugin_del(struct st_plugin_int *plugin, uint del_mask)
 {
   DBUG_ENTER("plugin_del");
   mysql_mutex_assert_owner(&LOCK_plugin);
+  del_mask|= PLUGIN_IS_UNINITIALIZED | PLUGIN_IS_DISABLED; // always use these
+  if (!(plugin->state & del_mask))
+    DBUG_VOID_RETURN;
   /* Free allocated strings before deleting the plugin. */
   plugin_vars_free_values(plugin->system_vars);
   restore_ptr_backup(plugin->nbackups, plugin->ptr_backup);
@@ -1339,7 +1337,7 @@ static void reap_plugins(void)
   mysql_mutex_lock(&LOCK_plugin);
 
   while ((plugin= *(--reap)))
-    plugin_del(plugin);
+    plugin_del(plugin, 0);
 
   my_afree(reap);
 }
@@ -1777,7 +1775,7 @@ int plugin_init(int *argc, char **argv, int flags)
       reaped_mandatory_plugin= TRUE;
     plugin_deinitialize(plugin_ptr, true);
     mysql_mutex_lock(&LOCK_plugin);
-    plugin_del(plugin_ptr);
+    plugin_del(plugin_ptr, 0);
   }
 
   mysql_mutex_unlock(&LOCK_plugin);
@@ -2065,12 +2063,14 @@ void plugin_shutdown(void)
     plugins= (struct st_plugin_int **) my_alloca(sizeof(void*) * (count+1));
 
     /*
-      If we have any plugins which did not die cleanly, we force shutdown
+      If we have any plugins which did not die cleanly, we force shutdown.
+      Don't re-deinit() plugins that failed deinit() earlier (already dying)
     */
     for (i= 0; i < count; i++)
     {
       plugins[i]= *dynamic_element(&plugin_array, i, struct st_plugin_int **);
-      /* change the state to ensure no reaping races */
+      if (plugins[i]->state == PLUGIN_IS_DYING)
+        plugins[i]->state= PLUGIN_IS_UNINITIALIZED;
       if (plugins[i]->state == PLUGIN_IS_DELETED)
         plugins[i]->state= PLUGIN_IS_DYING;
     }
@@ -2106,9 +2106,7 @@ void plugin_shutdown(void)
       if (plugins[i]->ref_count)
         sql_print_error("Plugin '%s' has ref_count=%d after shutdown.",
                         plugins[i]->name.str, plugins[i]->ref_count);
-      if (plugins[i]->state & PLUGIN_IS_UNINITIALIZED ||
-          plugins[i]->state & PLUGIN_IS_DISABLED)
-        plugin_del(plugins[i]);
+      plugin_del(plugins[i], PLUGIN_IS_DYING);
     }
 
     /*

sql/sql_show.cc

@@ -9810,23 +9810,17 @@ int initialize_schema_table(st_plugin_int *plugin)
 
 int finalize_schema_table(st_plugin_int *plugin)
 {
+  int deinit_status= 0;
   ST_SCHEMA_TABLE *schema_table= (ST_SCHEMA_TABLE *)plugin->data;
   DBUG_ENTER("finalize_schema_table");
 
   if (schema_table)
   {
     if (plugin->plugin->deinit)
-    {
-      DBUG_PRINT("info", ("Deinitializing plugin: '%s'", plugin->name.str));
-      if (plugin->plugin->deinit(NULL))
-      {
-        DBUG_PRINT("warning", ("Plugin '%s' deinit function returned error.",
-                               plugin->name.str));
-      }
-    }
+      deinit_status= plugin->plugin->deinit(NULL);
     my_free(schema_table);
   }
-  DBUG_RETURN(0);
+  DBUG_RETURN(deinit_status);
 }
 
 

storage/rocksdb/ha_rocksdb.cc

@@ -5203,9 +5203,6 @@ static rocksdb::Status check_rocksdb_options_compatibility(
   return status;
 }
 
-bool prevent_myrocks_loading= false;
-
-
 static int rocksdb_check_version(handlerton *hton,
                                  const char *path,
                                  const LEX_CUSTRING *version,
@@ -5226,14 +5223,6 @@ static int rocksdb_init_func(void *const p) {
 
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-  {
-    my_error(ER_INTERNAL_ERROR, MYF(0),
-             "Loading MyRocks plugin after it has been unloaded is not "
-             "supported. Please restart mariadbd");
-    DBUG_RETURN(1);
-  }
-
   if (rdb_check_rocksdb_corruption()) {
     // NO_LINT_DEBUG
     sql_print_error(
@@ -5794,8 +5783,6 @@ static int rocksdb_init_func(void *const p) {
 static int rocksdb_done_func(void *const p) {
   DBUG_ENTER_FUNC();
 
-  int error = 0;
-
   // signal the drop index thread to stop
   rdb_drop_idx_thread.signal(true);
 
@@ -5839,12 +5826,6 @@ static int rocksdb_done_func(void *const p) {
         "RocksDB: Couldn't stop the manual compaction thread: (errno=%d)", err);
   }
 
-  if (rdb_open_tables.count()) {
-    // Looks like we are getting unloaded and yet we have some open tables
-    // left behind.
-    error = 1;
-  }
-
   rdb_open_tables.free();
   /*
     destructors for static objects can be called at _exit(),
@@ -5896,7 +5877,7 @@ static int rocksdb_done_func(void *const p) {
     MariaDB: don't clear rocksdb_db_options and rocksdb_tbl_options.
     MyRocks' plugin variables refer to them.
 
-    The plugin cannot be loaded again (see prevent_myrocks_loading) but plugin
+    The plugin cannot be loaded again but plugin
     variables are processed before myrocks::rocksdb_init_func is invoked, so
     they must point to valid memory.
   */
@@ -5911,12 +5892,12 @@ static int rocksdb_done_func(void *const p) {
   my_error_unregister(HA_ERR_ROCKSDB_FIRST, HA_ERR_ROCKSDB_LAST);
 
   /*
-    Prevent loading the plugin after it has been loaded and then unloaded. This
-    doesn't work currently.
+    returning non-zero status from the plugin deinit function will prevent
+    the server from unloading the plugin. it will only be marked unusable.
+    This is needed here, because RocksDB cannot be fully unloaded
+    and reloaded (see sql_plugin.cc near STB_GNU_UNIQUE).
   */
-  prevent_myrocks_loading= true;
-
-  DBUG_RETURN(error);
+  DBUG_RETURN(1);
 }
 
 static inline void rocksdb_smart_seek(bool seek_backward,

storage/rocksdb/ha_rocksdb.h

@@ -1064,8 +1064,6 @@ std::string rdb_corruption_marker_file_name();
 
 const int MYROCKS_MARIADB_PLUGIN_MATURITY_LEVEL= MariaDB_PLUGIN_MATURITY_STABLE;
 
-extern bool prevent_myrocks_loading;
-
 void sql_print_verbose_info(const char *format, ...);
 
 }  // namespace myrocks

storage/rocksdb/mysql-test/rocksdb/r/mariadb_plugin.result

@@ -14,22 +14,18 @@ SELECT ENGINE, SUPPORT FROM INFORMATION_SCHEMA.ENGINES WHERE ENGINE='ROCKSDB';
 ENGINE	SUPPORT
 ROCKSDB	NO
 disconnect con1;
+select engine, support from information_schema.engines where engine='rocksdb';
+engine	support
+select plugin_name,plugin_status from information_schema.plugins where plugin_name='rocksdb';
+plugin_name	plugin_status
+ROCKSDB	INACTIVE
 #
 # MDEV-15686: Loading MyRocks plugin back after it has been unloaded causes a crash
 #
-call mtr.add_suppression("Plugin 'ROCKSDB.*' init function returned error.");
-call mtr.add_suppression("Plugin 'ROCKSDB.*' registration as a INFORMATION SCHEMA failed.");
-call mtr.add_suppression("Plugin 'ROCKSDB' registration as a STORAGE ENGINE failed");
-#
-# There are two possible scenarios:
-#  ha_rocksdb.{dll,so} is still loaded into mysqld's address space. Its
-#  global variables are in the state that doesn't allow it to be
-#  initialized back (this is what MDEV-15686 is about). This is handled 
-#  by intentionally returning an error from rocksdb_init_func.
-#
-#  The second case is when ha_rocksdb.{ddl,so} has been fully unloaded 
-#  and so it will be now loaded as if it happens for the first time.
-INSTALL SONAME 'ha_rocksdb';
-# Whatever happened on the previous step, restore things to the way they
-# were at testcase start.
-UNINSTALL SONAME 'ha_rocksdb';
+INSTALL PLUGIN rocksdb SONAME 'ha_rocksdb';
+ERROR HY000: Plugin 'rocksdb' already installed
+select engine, support from information_schema.engines where engine='rocksdb';
+engine	support
+select plugin_name,plugin_status from information_schema.plugins where plugin_name='rocksdb';
+plugin_name	plugin_status
+ROCKSDB	INACTIVE

storage/rocksdb/mysql-test/rocksdb/t/mariadb_plugin.test

@@ -31,29 +31,15 @@ disconnect con1;
 let $wait_condition= SELECT VARIABLE_VALUE=1 FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME='Threads_cached';
 --source include/wait_condition.inc
 
---echo #
---echo # MDEV-15686: Loading MyRocks plugin back after it has been unloaded causes a crash
---echo #
-call mtr.add_suppression("Plugin 'ROCKSDB.*' init function returned error.");
-call mtr.add_suppression("Plugin 'ROCKSDB.*' registration as a INFORMATION SCHEMA failed.");
-call mtr.add_suppression("Plugin 'ROCKSDB' registration as a STORAGE ENGINE failed");
+select engine, support from information_schema.engines where engine='rocksdb';
+select plugin_name,plugin_status from information_schema.plugins where plugin_name='rocksdb';
 
 --echo #
---echo # There are two possible scenarios:
-
---echo #  ha_rocksdb.{dll,so} is still loaded into mysqld's address space. Its
---echo #  global variables are in the state that doesn't allow it to be
---echo #  initialized back (this is what MDEV-15686 is about). This is handled 
---echo #  by intentionally returning an error from rocksdb_init_func.
+--echo # MDEV-15686: Loading MyRocks plugin back after it has been unloaded causes a crash
 --echo #
---echo #  The second case is when ha_rocksdb.{ddl,so} has been fully unloaded 
---echo #  and so it will be now loaded as if it happens for the first time.
-
---error 0,ER_INTERNAL_ERROR
-INSTALL SONAME 'ha_rocksdb';
 
---echo # Whatever happened on the previous step, restore things to the way they
---echo # were at testcase start.
---error 0,ER_SP_DOES_NOT_EXIST
-UNINSTALL SONAME 'ha_rocksdb';
+--error ER_PLUGIN_INSTALLED
+INSTALL PLUGIN rocksdb SONAME 'ha_rocksdb';
 
+select engine, support from information_schema.engines where engine='rocksdb';
+select plugin_name,plugin_status from information_schema.plugins where plugin_name='rocksdb';