Hi Sergei, Thanks for the comments, which I address below. The updated commit for review is posted on the jira ticket. On Sat 2023-07-01 17:39:59 +0200, Sergei Golubchik wrote:
Hi, Yuchen,
On Jul 01, Yuchen Pei wrote:
revision-id: 8b5de389ab1 (mariadb-10.9.5-17-g8b5de389ab1) parent(s): d8997f875e2 author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-06-13 20:08:28 +1000 message:
MDEV-31400 Simple plugin dependency resolution
We introduce simple plugin dependency. A plugin init function may return HA_ERR_RETRY_INIT. If this happens during server startup when the server is trying to initialise all plugins, the failed plugins will be retried, until no more plugins succeed in initialisation or want to be retried.
This will fix spider init bugs which is caused in part by its dependency on Aria for initialisation.
The reason we need a new return code, instead of treating every failure as a request for retry, is that it may be impossible to clean up after a failed plugin initialisation. Take InnoDB for example, it has a global variable `buf_page_cleaner_is_active`, which may not satisfy an assertion during a second initialisation try, probably because InnoDB does not expect the initialisation to be called twice. A test that may fail because of this is `encryption.corrupted_during_recovery`, see for example[1], which is tested at 73835f64b7fc245d38812380685aca03bef72bb5, a previous commit
again, commits that you are going to push into the main branche should not refer to commits that you aren't going to push. these are dangling references leading nowhere.
Done
where we retry every failed plugin.
[1] https://buildbot.mariadb.org/#/builders/369/builds/10107/steps/7/logs/stdio
nor references to buildbot logs, they're very short lived, while I can see commits in the server git repo going to 2000.
Done.
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 5a077a934ac..92d74aa51e8 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -1435,6 +1435,49 @@ 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 and may "
please, don't write "may". It's saying "eh, I don't know, you, user, figure it out". But you should know better than the user. So either say "will be retried" or "won't be retried" (in which case, you can just use the error message below and not mention retry at all).
Done. I removed the "may ..." part, and kept it a warning because the init may succeed on potential retries.
+ "be retried provided it is being loaded with " + "plugin-load[-add]", + 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) @@ -1737,32 +1738,71 @@ 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= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); + *(retry++)= NULL;
for(;;) { + /* Number of plugins that is successfully initialised in a round */ + int num_initialized; + do + { + num_initialized= 0; + /* If any plugins failed and requested a retry, clean up before + retry */ + while ((plugin_ptr= *(--retry))) + plugin_ptr->state= PLUGIN_IS_TO_BE_RETRIED;
Why do you need a special state for that? You haven't fixed the SHOW PLUGINS command to show it correctly. And I didn't check other conditions, may be some of them need to take the new state into account too, I don't know.
Wouldn't it be easier to have a special retry look after the main for() without all its complexity and without a new state. Like (pseudocode, and note that here I've changed the creative trick of walking retry[] backwards until NULL, so now it iterated forward until end):
while (retry_start < retry_end) { retry_ptr= retry= retry_start; while ((plugin_ptr= *(retry++))) { mysql_mutex_unlock(&LOCK_plugin); error= plugin_do_initialize(plugin_ptr, state); mysql_mutex_lock(&LOCK_plugin); if (error == HA_ERR_RETRY_INIT) *(retry_ptr++)= plugin_ptr; else if (error) *(reap++)= plugin_ptr; } if (retry == retry_ptr) while (retry_ptr > retry_start) *(reap++)= *(--retry_ptr); retry_end= retry_ptr; }
Nice, done.
+ retry++; for (i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) { HASH *hash= plugin_hash + plugin_type_initialization_order[i]; for (uint idx= 0; idx < hash->records; idx++) { plugin_ptr= (struct st_plugin_int *) my_hash_element(hash, idx); + if (plugin_ptr->state == PLUGIN_IS_UNINITIALIZED || + plugin_ptr->state == PLUGIN_IS_TO_BE_RETRIED) + { + int error; if (plugin_ptr->state == PLUGIN_IS_UNINITIALIZED) { 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)) + error= plugin_initialize(&tmp_root, plugin_ptr, argc, argv, + opts_only); + } else { + uint state= plugin_ptr->state; + mysql_mutex_unlock(&LOCK_plugin); + error= plugin_do_initialize(plugin_ptr, state); + mysql_mutex_lock(&LOCK_plugin); + plugin_ptr->state= state; + } + if (error) + { plugin_ptr->state= PLUGIN_IS_DYING; + /* The plugin wants a retry of the initialisation, + possibly due to dependency on other plugins */ + if (unlikely(error == HA_ERR_RETRY_INIT)) + *(retry++)= plugin_ptr; + else *(reap++)= plugin_ptr; } + else + num_initialized++; } } } + /* Only retry if at least one plugin has been initialised + successfully and at least one has requested a retry during this + round */ + } while (num_initialized > 0 && *(retry - 1));
/* load and init plugins from the plugin table (unless done already) */ if (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE) @@ -1775,8 +1815,11 @@ 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 */ + while ((plugin_ptr= *(--retry))) + *(reap++) = plugin_ptr;
I already have that in my pseudocode above
Ack.
while ((plugin_ptr= *(--reap))) { mysql_mutex_unlock(&LOCK_plugin);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Best, Yuchen