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 secur...@mariadb.org
Best,
Yuchen
_______________________________________________
Mailing list: $list_owner
Post to : $list_post
Unsubscribe : $list_unsubscribe
More help : $list_help
