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