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 secur...@mariadb.org Best, Yuchen _______________________________________________ developers mailing list -- developers@lists.mariadb.org To unsubscribe send an email to developers-le...@lists.mariadb.org
