[MariaDB developers]Re: a2e71bc8e89: MDEV-22979 MDEV-31400 Fixing spider init bugs
Hi Sergei, Thanks for the comments. I have addressed them and the url of the new patch can be found in the new request for review comment in the ticket. On Mon 2023-06-12 18:07:03 +0200, Sergei Golubchik wrote:
Hi, Yuchen,
See comments inline. Here I'm only reviewing MDEV-31400, please, tell me if you'd like me to review spider part too
Given that Alexey (cc'd) has already been working on the review of the spider init bug fix, I think it makes sense to ask Alexey to continue on the spider part, which I just did on MDEV-22979. Given that the spider part requires the patch for MDEV-31400 to work, I have the spider part on top of the init dependency part.
On Jun 12, Yuchen Pei wrote:
revision-id: a2e71bc8e89 (mariadb-10.9.5-19-ga2e71bc8e89) parent(s): cb0e0c915f6 author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-06-08 16:13:56 +1000 message:
MDEV-22979 MDEV-31400 Fixing spider init bugs
please, make a separate commit for MDEV-31400
Done.
diff --git a/sql/handler.cc b/sql/handler.cc index a12e9ea18f5..60080f1da6a 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -646,10 +648,14 @@ int ha_initialize_handlerton(st_plugin_int *plugin) hton->slot= HA_SLOT_UNDEF; /* Historical Requirement */ plugin->data= hton; // shortcut for the future - if (plugin->plugin->init && plugin->plugin->init(hton)) + if (plugin->plugin->init && (ret= plugin->plugin->init(hton))) { - sql_print_error("Plugin '%s' init function returned error.", - plugin->name.str); + if (unlikely(ret == -1)) + sql_print_warning("Plugin '%s' init function returned error but may be retried.", + plugin->name.str); + else + sql_print_error("Plugin '%s' init function returned error.", + plugin->name.str);
let's treat all plugins identically. Meaning, in particular, no warnings or errors in ha_initialize_handlerton(). They belong to sql_plugin.cc
Done.
goto err; }
diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 32392ab882e..caf85736770 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -365,6 +365,7 @@ bool opt_disable_networking=0, opt_skip_show_db=0; bool opt_skip_name_resolve=0; my_bool opt_character_set_client_handshake= 1; bool opt_endinfo, using_udf_functions; +bool udf_initialized= 0;
I don't think you need that. You can use exising mysqld_server_started.
That won't work, because mysqld_server_started will only be turned on very late, towards the end of mysqld_main(). If spider is initialised between udf_init() and mysqld_server_started is 1 (for example in an init-file), then the udf will fail. See https://github.com/MariaDB/server/commit/91176a7be7c where the test udf_mysql_func_early fails with "query 'SELECT SPIDER_DIRECT_SQL('select * from tbl_a', 'results', 'srv "s_2_1", database "auto_test_remote"')' failed: ER_SP_DOES_NOT_EXIST (1305): FUNCTION auto_test_local.SPIDER_DIRECT_SQL does not exist" because of this. Now, I assume MDEV-31401, the task to include the udf insertion trick in early calls CREATE FUNCTION, will fix this issue without introduce any new global booleans because it should has access to the internal variable `initialized`, though I do not know how complex or doable this task is. So I guess there's a trade-off here between fixing the spider init bugs now and investigating MDEV-31401. Would you like me to explore MDEV-31401 first?
my_bool locked_in_memory; bool opt_using_transactions; bool volatile abort_loop; diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 5a077a934ac..e564ab5a38d 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -1462,10 +1462,16 @@ static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin,
if (plugin_type_initialize[plugin->plugin->type]) { - if ((*plugin_type_initialize[plugin->plugin->type])(plugin)) + ret= (*plugin_type_initialize[plugin->plugin->type])(plugin); + if (ret) { - sql_print_error("Plugin '%s' registration as a %s failed.", - plugin->name.str, plugin_type_names[plugin->plugin->type].str); + /* Plugin init failed but requested a retry if possible */ + if (unlikely(ret == -1)) + sql_print_warning("Plugin '%s' registration as a %s failed but may be retried.", + 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);
1. "may be" is confusing, we should be less vague in the error messages
Done. I have expanded it a bit more to the following but we still really use "will be" here without either losing accuracy or being overly verbose. Even if it fails and returns the retry code, the retry will not happen if no new plugins are successfully initialised this round. --8<---------------cut here---------------start------------->8--- sql_print_warning("Plugin '%s' registration as a %s failed and may " "be retried provided it is being loaded with " "plugin-load[-add]", plugin->name.str, plugin_type_names[plugin->plugin->type].str); --8<---------------cut here---------------end--------------->8---
2. about (ret == -1). I looked at what plugin init functions return now: 33 plugins don't have an init function 76 plugins always return 0 13 plugins return 0 or 1 3 plugins return 0 or -1 3 plugins return 0 or HA_ERR_INITIALIZATION, HA_ERR_OUT_OF_MEM and innodb inconsistently return 0, HA_ERR_xxx as above, or 1
so, I suggest to introduce, like, HA_ERR_DEPENDENCIES (or whatever) and only retry plugins that return that specific error code.
Done. The HA_ERR_ namespace is tightly packed and bounded by HA_ERR_FIRST (120) and HA_ERR_LAST (198), and I crammed in a #define HA_ERR_RETRY_INIT 129.
goto err; } } @@ -1462,8 +1462,14 @@ static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin,
if (plugin_type_initialize[plugin->plugin->type]) { - if ((*plugin_type_initialize[plugin->plugin->type])(plugin)) + ret= (*plugin_type_initialize[plugin->plugin->type])(plugin);
there's a second branch below, with plugin->plugin->init(), please, handle it too. May be, do as with deinit?
plugin_type_init deinit= plugin_type_deinitialize[plugin->plugin->type]; if (!deinit) deinit= (plugin_type_init)(plugin->plugin->deinit);
if (deinit && deinit(plugin))
Less code duplication here.
Done.
+ if (ret) { + /* Plugin init failed but requested a retry if possible */ + if (unlikely(ret == -1)) + sql_print_warning("Plugin '%s' registration as a %s failed but may be retried.", + 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); goto err; @@ -1737,11 +1743,34 @@ 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))) + { + mysql_mutex_unlock(&LOCK_plugin); + plugin_deinitialize(plugin_ptr, true); + mysql_mutex_lock(&LOCK_plugin); + /** Needed to satisfy assertions in `test_plugin_options()` */
Hmm, I don't think it'll work. I mean, test_plugin_options() - as far as I remember - will remove processed options from the command line array, meaning you cannot call it twice for the same plugin.
May be it'd be better to repeat only plugin init function call and not the complete initialize/deinitialize cycle?
Done. I separated out a plugin_do_initialize() function that skips the test_options() part and largely mirrors the plugin_deinitialize() function, and calls this function instead of the full plugin_initialize() during a retry. I also made sure the code path outside of plugin_init() does not change, i.e. if plugin_initialize() is called from an INSTALL statement. The contract of init and deinit is not clear, but I suppose it is a reasonable expectation that if a plugin init wants to be retried, it will clean up before returning init instead of relying the server to call the deinit before retrying.
+ my_afree(plugin_ptr->ptr_backup); + plugin_ptr->ptr_backup= NULL; + plugin_ptr->nbackups= 0; + plugin_ptr->state= PLUGIN_IS_UNINITIALIZED; + } + retry++; for (i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) { HASH *hash= plugin_hash + plugin_type_initialization_order[i];
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Best, Yuchen
Hi, Yuchen, On Jun 13, Yuchen Pei wrote:
Hi Sergei,
Thanks for the comments. I have addressed them and the url of the new patch can be found in the new request for review comment in the ticket.
Thanks. I'll send a separate email with comments on the new patch. Here I'll just reply to your email.
diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 32392ab882e..caf85736770 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -365,6 +365,7 @@ bool opt_disable_networking=0, opt_skip_show_db=0; bool opt_skip_name_resolve=0; my_bool opt_character_set_client_handshake= 1; bool opt_endinfo, using_udf_functions; +bool udf_initialized= 0;
I don't think you need that. You can use exising mysqld_server_started.
That won't work, because mysqld_server_started will only be turned on very late, towards the end of mysqld_main(). If spider is initialised between udf_init() and mysqld_server_started is 1 (for example in an init-file), then the udf will fail. See https://github.com/MariaDB/server/commit/91176a7be7c where the test udf_mysql_func_early fails with "query 'SELECT SPIDER_DIRECT_SQL('select * from tbl_a', 'results', 'srv "s_2_1", database "auto_test_remote"')' failed: ER_SP_DOES_NOT_EXIST (1305): FUNCTION auto_test_local.SPIDER_DIRECT_SQL does not exist" because of this.
Now, I assume MDEV-31401, the task to include the udf insertion trick in early calls CREATE FUNCTION, will fix this issue without introduce any new global booleans because it should has access to the internal variable `initialized`, though I do not know how complex or doable this task is. So I guess there's a trade-off here between fixing the spider init bugs now and investigating MDEV-31401. Would you like me to explore MDEV-31401 first?
As you like. You can do MDEV-31401 or work around the issue with, for example BEGIN NOT ATOMIC DECLARE EXIT HANDLER FOR 1123 INSERT mysql.func (...), (...), ...; CREATE FUNCTION ...; CREATE FUNCTION ...; CREATE FUNCTION ...; CREATE FUNCTION ...; END; just don't add new spider-specific global variables to the server.
my_bool locked_in_memory; bool opt_using_transactions; bool volatile abort_loop;
2. about (ret == -1). I looked at what plugin init functions return now: 33 plugins don't have an init function 76 plugins always return 0 13 plugins return 0 or 1 3 plugins return 0 or -1 3 plugins return 0 or HA_ERR_INITIALIZATION, HA_ERR_OUT_OF_MEM and innodb inconsistently return 0, HA_ERR_xxx as above, or 1
so, I suggest to introduce, like, HA_ERR_DEPENDENCIES (or whatever) and only retry plugins that return that specific error code.
Done. The HA_ERR_ namespace is tightly packed and bounded by HA_ERR_FIRST (120) and HA_ERR_LAST (198), and I crammed in a #define HA_ERR_RETRY_INIT 129.
It's not tightly packed, HA_ERR_LAST is just a high water mark. You can add a new HA_ERR_ at the end and increment HA_ERR_LAST. But 129 works too, that's fine.
goto err; } }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, Thanks for your comments.
As you like. You can do MDEV-31401 or work around the issue with, for example
BEGIN NOT ATOMIC DECLARE EXIT HANDLER FOR 1123 INSERT mysql.func (...), (...), ...; CREATE FUNCTION ...; CREATE FUNCTION ...; CREATE FUNCTION ...; CREATE FUNCTION ...; END;
just don't add new spider-specific global variables to the server.
How about renaming and exporting an existing variable, like in https://github.com/MariaDB/server/commit/74afa221807? Does that also count as adding a new spider-specific global variable to the server?
Done. The HA_ERR_ namespace is tightly packed and bounded by HA_ERR_FIRST (120) and HA_ERR_LAST (198), and I crammed in a #define HA_ERR_RETRY_INIT 129.
It's not tightly packed, HA_ERR_LAST is just a high water mark. You can add a new HA_ERR_ at the end and increment HA_ERR_LAST.
But 129 works too, that's fine.
Ack. Best, Yuchen
On Mon 2023-07-03 16:18:34 +1000, Yuchen Pei wrote:
Hi Sergei,
Thanks for your comments.
As you like. You can do MDEV-31401 or work around the issue with, for example
BEGIN NOT ATOMIC DECLARE EXIT HANDLER FOR 1123 INSERT mysql.func (...), (...), ...; CREATE FUNCTION ...; CREATE FUNCTION ...; CREATE FUNCTION ...; CREATE FUNCTION ...; END;
just don't add new spider-specific global variables to the server.
How about renaming and exporting an existing variable, like in https://github.com/MariaDB/server/commit/74afa221807? Does that also count as adding a new spider-specific global variable to the server?
In any case I have incorporated your suggestions in https://github.com/MariaDB/server/commit/528c7fa1c24 Best, Yuchen
participants (2)
-
Sergei Golubchik -
Yuchen Pei