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

10 files changed, 57 insertions, 150 deletions

diff --git a/sql/encryption.cc b/sql/encryption.cc
index 13239b91910..3c7ba2e997b 100644
--- a/sql/encryption.cc
+++ b/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;
 }
 
 /******************************************************************
diff --git a/sql/handler.cc b/sql/handler.cc
index 3e88ec39b68..8c9bb983d4f 100644
--- a/sql/handler.cc
+++ b/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);
 }
 
 
diff --git a/sql/sql_audit.cc b/sql/sql_audit.cc
index 3e9379ebe33..cbf3f971088 100644
--- a/sql/sql_audit.cc
+++ b/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;
 }
 
 
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc
index d581cd049e8..d493c69e38a 100644
--- a/sql/sql_plugin.cc
+++ b/sql/sql_plugin.cc
@@ -1267,13 +1267,8 @@ 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.",
@@ -1281,10 +1276,13 @@ static void plugin_deinitialize(struct st_plugin_int *plugin, bool ref_check)
   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);
     }
 
     /*
diff --git a/sql/sql_show.cc b/sql/sql_show.cc
index 536135a392b..d01f84fe7d1 100644
--- a/sql/sql_show.cc
+++ b/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);
 }
 
 
diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc
index a8a3a27ed4f..a56f47d4c1b 100644
--- a/storage/rocksdb/ha_rocksdb.cc
+++ b/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,
diff --git a/storage/rocksdb/ha_rocksdb.h b/storage/rocksdb/ha_rocksdb.h
index 6d2d877c8ec..0358508632f 100644
--- a/storage/rocksdb/ha_rocksdb.h
+++ b/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
diff --git a/storage/rocksdb/mysql-test/rocksdb/r/mariadb_plugin.result b/storage/rocksdb/mysql-test/rocksdb/r/mariadb_plugin.result
index 6d6cb1db54e..ab13976b742 100644
--- a/storage/rocksdb/mysql-test/rocksdb/r/mariadb_plugin.result
+++ b/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
diff --git a/storage/rocksdb/mysql-test/rocksdb/t/mariadb_plugin.test b/storage/rocksdb/mysql-test/rocksdb/t/mariadb_plugin.test
index 0cf56c0cbd5..8f30d7c42b6 100644
--- a/storage/rocksdb/mysql-test/rocksdb/t/mariadb_plugin.test
+++ b/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';
diff --git a/storage/rocksdb/rdb_i_s.cc b/storage/rocksdb/rdb_i_s.cc
index 5350ec3bce9..a3d284dfa64 100644
--- a/storage/rocksdb/rdb_i_s.cc
+++ b/storage/rocksdb/rdb_i_s.cc
@@ -145,9 +145,6 @@ static int rdb_i_s_cfstats_fill_table(
 static int rdb_i_s_cfstats_init(void *p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   DBUG_ASSERT(p != nullptr);
 
   my_core::ST_SCHEMA_TABLE *schema;
@@ -241,9 +238,6 @@ static int rdb_i_s_dbstats_fill_table(
 static int rdb_i_s_dbstats_init(void *const p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   DBUG_ASSERT(p != nullptr);
 
   my_core::ST_SCHEMA_TABLE *schema;
@@ -344,8 +338,6 @@ static int rdb_i_s_perf_context_fill_table(
 static int rdb_i_s_perf_context_init(void *const p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
   DBUG_ASSERT(p != nullptr);
 
   my_core::ST_SCHEMA_TABLE *schema;
@@ -413,9 +405,6 @@ static int rdb_i_s_perf_context_global_fill_table(
 static int rdb_i_s_perf_context_global_init(void *const p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   DBUG_ASSERT(p != nullptr);
 
   my_core::ST_SCHEMA_TABLE *schema;
@@ -1045,9 +1034,6 @@ static int rdb_i_s_ddl_fill_table(my_core::THD *const thd,
 static int rdb_i_s_ddl_init(void *const p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   my_core::ST_SCHEMA_TABLE *schema;
 
   DBUG_ASSERT(p != nullptr);
@@ -1063,9 +1049,6 @@ static int rdb_i_s_ddl_init(void *const p) {
 static int rdb_i_s_cfoptions_init(void *const p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   DBUG_ASSERT(p != nullptr);
 
   my_core::ST_SCHEMA_TABLE *schema;
@@ -1081,9 +1064,6 @@ static int rdb_i_s_cfoptions_init(void *const p) {
 static int rdb_i_s_global_info_init(void *const p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   DBUG_ASSERT(p != nullptr);
 
   my_core::ST_SCHEMA_TABLE *schema;
@@ -1101,9 +1081,6 @@ static int rdb_i_s_compact_stats_init(void *p) {
 
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   DBUG_ASSERT(p != nullptr);
 
   schema = reinterpret_cast<my_core::ST_SCHEMA_TABLE *>(p);
@@ -1438,9 +1415,6 @@ static int rdb_i_s_index_file_map_fill_table(
 static int rdb_i_s_index_file_map_init(void *const p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   DBUG_ASSERT(p != nullptr);
 
   my_core::ST_SCHEMA_TABLE *schema;
@@ -1523,9 +1497,6 @@ static int rdb_i_s_lock_info_fill_table(
 static int rdb_i_s_lock_info_init(void *const p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   DBUG_ASSERT(p != nullptr);
 
   my_core::ST_SCHEMA_TABLE *schema;
@@ -1651,9 +1622,6 @@ static int rdb_i_s_trx_info_fill_table(
 static int rdb_i_s_trx_info_init(void *const p) {
   DBUG_ENTER_FUNC();
 
-  if (prevent_myrocks_loading)
-    DBUG_RETURN(1);
-
   DBUG_ASSERT(p != nullptr);
 
   my_core::ST_SCHEMA_TABLE *schema;
@@ -1781,7 +1749,8 @@ static int rdb_i_s_deadlock_info_init(void *const p) {
 
 static int rdb_i_s_deinit(void *p MY_ATTRIBUTE((__unused__))) {
   DBUG_ENTER_FUNC();
-  DBUG_RETURN(0);
+  /* see the comment at the end of rocksdb_done_func() */
+  DBUG_RETURN(1);
 }
 
 static struct st_mysql_information_schema rdb_i_s_info = {
@@ -1955,7 +1924,7 @@ struct st_maria_plugin rdb_i_s_lock_info = {
     "RocksDB lock information",
     PLUGIN_LICENSE_GPL,
     rdb_i_s_lock_info_init,
-    nullptr,
+    rdb_i_s_deinit,
     0x0001,  /* version number (0.1) */
     nullptr, /* status variables */
     nullptr, /* system variables */
@@ -1971,7 +1940,7 @@ struct st_maria_plugin rdb_i_s_trx_info = {
     "RocksDB transaction information",
     PLUGIN_LICENSE_GPL,
     rdb_i_s_trx_info_init,
-    nullptr,
+    rdb_i_s_deinit,
     0x0001,  /* version number (0.1) */
     nullptr, /* status variables */
     nullptr, /* system variables */
@@ -1987,7 +1956,7 @@ struct st_maria_plugin rdb_i_s_deadlock_info = {
     "RocksDB transaction information",
     PLUGIN_LICENSE_GPL,
     rdb_i_s_deadlock_info_init,
-    nullptr,
+    rdb_i_s_deinit,
     0x0001,  /* version number (0.1) */
     nullptr, /* status variables */
     nullptr, /* system variables */