Re: 534031b77e0: MDEV-31400 Simple plugin dependency resolution
![](https://secure.gravatar.com/avatar/39b623a1559cf9c69ac3d9d4fb44e7fe.jpg?s=120&d=mm&r=g)
Hi, Yuchen, Thanks. Conceptually all good, the approach is correct. See comments below - about the state and about user visible warnings, nothing major. On Jul 18, Yuchen Pei wrote:
revision-id: 534031b77e0 (mariadb-10.9.5-17-g534031b77e0) parent(s): d8997f875e2 author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-07-03 17:54:56 +1000 message:
MDEV-31400 Simple plugin dependency resolution
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 5a077a934ac..15717352040 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -1435,6 +1435,47 @@ void plugin_unlock_list(THD *thd, plugin_ref *list, size_t count) DBUG_VOID_RETURN; }
+static int plugin_do_initialize(struct st_plugin_int *plugin, uint &state) +{ + DBUG_ENTER("plugin_do_initialize"); + mysql_mutex_assert_not_owner(&LOCK_plugin); + plugin_type_init init= plugin_type_initialize[plugin->plugin->type]; + if (!init) + init= (plugin_type_init) plugin->plugin->init; + if (init) + if (int ret= init(plugin)) + { + /* Plugin init failed but requested a retry if possible */ + if (unlikely(ret== HA_ERR_RETRY_INIT)) + sql_print_warning("Plugin '%s' registration as a %s failed.", + plugin->name.str, plugin_type_names[plugin->plugin->type].str); + else + sql_print_error("Plugin '%s' registration as a %s failed.", + plugin->name.str, plugin_type_names[plugin->plugin->type].str); + DBUG_RETURN(ret); + } + state= PLUGIN_IS_READY; // plugin->init() succeeded + + if (plugin->plugin->status_vars) + { + /* + historical ndb behavior caused MySQL plugins to specify + status var names in full, with the plugin name prefix. + this was never fixed in MySQL. + MariaDB fixes that but supports MySQL style too. + */ + SHOW_VAR *show_vars= plugin->plugin->status_vars; + SHOW_VAR tmp_array[2]= {{plugin->plugin->name, + (char *) plugin->plugin->status_vars, SHOW_ARRAY}, + {0, 0, SHOW_UNDEF}}; + if (strncasecmp(show_vars->name, plugin->name.str, plugin->name.length)) + show_vars= tmp_array; + + if (add_status_vars(show_vars)) + DBUG_RETURN(1); + } + DBUG_RETURN(0); +}
static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, int *argc, char **argv, bool options_only) @@ -1595,7 +1594,7 @@ int plugin_init(int *argc, char **argv, int flags) size_t i; struct st_maria_plugin **builtins; struct st_maria_plugin *plugin; - struct st_plugin_int tmp, *plugin_ptr, **reap; + struct st_plugin_int tmp, *plugin_ptr, **reap, **retry_end, **retry_start; MEM_ROOT tmp_root; bool reaped_mandatory_plugin= false; bool mandatory= true; @@ -1737,11 +1736,16 @@ int plugin_init(int *argc, char **argv, int flags) */
mysql_mutex_lock(&LOCK_plugin); + /* List of plugins to reap */ reap= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); *(reap++)= NULL; + /* List of plugins to retry */ + retry_start= retry_end= + (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*));
for(;;) { + int error; for (i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) { HASH *hash= plugin_hash + plugin_type_initialization_order[i]; @@ -1753,16 +1757,46 @@ int plugin_init(int *argc, char **argv, int flags) bool plugin_table_engine= lex_string_eq(&plugin_table_engine_name, &plugin_ptr->name); bool opts_only= flags & PLUGIN_INIT_SKIP_INITIALIZATION && - (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE || - !plugin_table_engine); - if (plugin_initialize(&tmp_root, plugin_ptr, argc, argv, opts_only)) + (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE || !plugin_table_engine); + error= plugin_initialize(&tmp_root, plugin_ptr, argc, argv, + opts_only); + if (error) { plugin_ptr->state= PLUGIN_IS_DYING; - *(reap++)= plugin_ptr; + /* The plugin wants a retry of the initialisation, + possibly due to dependency on other plugins */ + if (unlikely(error == HA_ERR_RETRY_INIT)) + *(retry_end++)= plugin_ptr; + else + *(reap++)= plugin_ptr; } } } } + /* Retry plugins that asked for it */ + while (retry_start < retry_end) + { + st_plugin_int **to_re_retry, **retrying; + for (to_re_retry= retrying= retry_start; retrying < retry_end; retrying++) + { + plugin_ptr= *retrying; + uint state= plugin_ptr->state; + mysql_mutex_unlock(&LOCK_plugin);
you might want to add here sql_print_information("plugin XXX registration is retried") otherwise it would be confusing to see "plugin XXX registration failed" and then see the plugin perferctly initialized and usable. Alternatively (preferable, even) don't print "plugin registration failed" above, print it when adding a plugin to the reap list. So that the user won't see fail/retry loop at all, this is the inner working of the plugin subsystem, no user serviceable parts inside.
+ error= plugin_do_initialize(plugin_ptr, state); + mysql_mutex_lock(&LOCK_plugin); + plugin_ptr->state= state;
why do you need to manipulate (save/restore) the state here? Is that even correct? You set the state to PLUGIN_IS_DYING earlier and plugin_do_initialize() an change it to PLUGIN_IS_READY on success. You wouldn't want to change PLUGIN_IS_READY to PLUGIN_IS_DYING, would you? If this is the case, then you don't need to save the state at all, instead you need something like if (error) plugin_ptr->state= PLUGIN_IS_DYING;
+ if (error == HA_ERR_RETRY_INIT) + *(to_re_retry++)= plugin_ptr; + else if (error) + *(reap++)= plugin_ptr; + } + /* If the retry list has not changed, i.e. if all retry attempts + result in another retry request, empty the retry list */ + if (to_re_retry == retry_end) + while (to_re_retry > retry_start) + *(reap++)= *(--to_re_retry); + retry_end= to_re_retry; + }
/* load and init plugins from the plugin table (unless done already) */ if (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE) @@ -1775,7 +1809,8 @@ int plugin_init(int *argc, char **argv, int flags) }
/* - Check if any plugins have to be reaped + Merge the retry list to the reap list, then reap the failed + plugins. Note that during the merge we reverse the order in retry
seems like this comment is obsolete
*/ while ((plugin_ptr= *(--reap))) { @@ -1788,6 +1823,7 @@ int plugin_init(int *argc, char **argv, int flags) }
mysql_mutex_unlock(&LOCK_plugin); + my_afree(retry_start); my_afree(reap); if (reaped_mandatory_plugin && !opt_help) goto err;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
![](https://secure.gravatar.com/avatar/b64f96b50a4798e55441d41387e1d35d.jpg?s=120&d=mm&r=g)
Hi Sergei, Thanks for the comments, I have addressed them and as usual the url to the updated commit is in the latest comment in the jira ticket. On Wed 2023-07-19 18:37:18 +0200, Sergei Golubchik wrote:
Hi, Yuchen,
Thanks. Conceptually all good, the approach is correct. See comments below - about the state and about user visible warnings, nothing major.
On Jul 18, Yuchen Pei wrote:
+ /* Retry plugins that asked for it */ + while (retry_start < retry_end) + { + st_plugin_int **to_re_retry, **retrying; + for (to_re_retry= retrying= retry_start; retrying < retry_end; retrying++) + { + plugin_ptr= *retrying; + uint state= plugin_ptr->state; + mysql_mutex_unlock(&LOCK_plugin);
you might want to add here
sql_print_information("plugin XXX registration is retried")
otherwise it would be confusing to see "plugin XXX registration failed" and then see the plugin perferctly initialized and usable.
Alternatively (preferable, even) don't print "plugin registration failed" above, print it when adding a plugin to the reap list. So that the user won't see fail/retry loop at all, this is the inner working of the plugin subsystem, no user serviceable parts inside.
Done (following the "Alternatively" suggestion).
+ error= plugin_do_initialize(plugin_ptr, state); + mysql_mutex_lock(&LOCK_plugin); + plugin_ptr->state= state;
why do you need to manipulate (save/restore) the state here? Is that even correct? You set the state to PLUGIN_IS_DYING earlier and plugin_do_initialize() an change it to PLUGIN_IS_READY on success. You wouldn't want to change PLUGIN_IS_READY to PLUGIN_IS_DYING, would you?
In this case it does not restore the state to PLUGIN_IS_DYING, because `state', passed as a reference, is mutated inside `plugin_do_initialize()' to PLUGIN_IS_READY. This is a pattern I copied from somewhere else, e.g. `plugin_initialize()' (see below), with the understanding that `plugin_ptr->state' may be mutated in other threads between the unlocking and locking of the mutex, and by doing this we keep the state consistent in the current thread. --8<---------------cut here---------------start------------->8--- static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, int *argc, char **argv, bool options_only) { ... uint state= plugin->state; DBUG_ASSERT(state == PLUGIN_IS_UNINITIALIZED); mysql_mutex_unlock(&LOCK_plugin); ... mysql_mutex_lock(&LOCK_plugin); plugin->state= state; --8<---------------cut here---------------end--------------->8---
If this is the case, then you don't need to save the state at all, instead you need something like
if (error) plugin_ptr->state= PLUGIN_IS_DYING;
/* - Check if any plugins have to be reaped + Merge the retry list to the reap list, then reap the failed + plugins. Note that during the merge we reverse the order in retry
seems like this comment is obsolete
Done. BTW the retry mechanism is currently only tested by the spider change[1] on top of it, and the spider change can only be applied to 10.9+, until MDEV-27595 Backport SQL service is done. In that sense the change introduced in this ticket to versions less than 10.9 (needed for fixing the spider init bugs for those versions) is blocked by that ticket. So my question is: once this change is approved, do we wait for MDEV-27595 to be fixed before pushing (to 10.4), OR do we do a push to 10.9 first, then to 10.4 after MDEV-27595 is pushed, OR do we just push to 10.4 without waiting for MDEV-27595? [1] https://github.com/MariaDB/server/commit/2d58a8e941e Best, Yuchen
participants (2)
-
Sergei Golubchik
-
Yuchen Pei