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