Hi, Yuchen, On Jan 05, Yuchen Pei wrote:

revision-id: 014167880d2 (mariadb-10.10.4-52-g014167880d2) parent(s): 21822625aa6 author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-11-08 13:10:38 +1100 message:

MDEV-32559 ensure spider alter table init queries are executed after ddl recovery

If spider was initialised in init_server_components(), i.e. with --plugin-load-add, then all alter table init queries are to be executed in the signal_ddl_recovery_done() handlerton callback. Since this is part of the spider plugin init, we update the callback signature to return an int, and deinit the plugin when it fails the callback.

diff --git a/sql/mysqld.cc b/sql/mysqld.cc index a3971a00a59..a922562d043 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -5159,6 +5159,14 @@ static int init_server_components() if (!opt_abort && ddl_log_initialize()) unireg_abort(1);

+ /* Plugin initialisation done here should defer any ALTER TABLE + queries to after the ddl recovery is done, in the + signal_ddl_recovery_done() callback called by + ha_signal_ddl_recovery_done(). As such, between the plugin_init() + call and the ha_signal_ddl_recovery_done() call below only things + related to prepare for recovery should be done and nothing else, and + definitely not anything assuming that all plugins have been + initialised. */

Better put this comment in plugin.h near the (*init)(void*) declaration. This is part of the (*init)() callback documentation it should be there, not in some random .cc file that plugin authors shouldn't be required to look at.

if (plugin_init(&remaining_argc, remaining_argv, (opt_noacl ? PLUGIN_INIT_SKIP_PLUGIN_TABLE : 0) | (opt_abort ? PLUGIN_INIT_SKIP_INITIALIZATION : 0))) diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 3576730723e..85a8f14a6da 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -2553,9 +2554,15 @@ bool plugin_foreach_with_mask(THD *thd, plugin_foreach_func *func,

for (idx= 0; idx < total; idx++) { - /* It will stop iterating on first engine error when "func" returns TRUE */ + /* It will stop iterating on first engine error or mark DELETED if + reap_on_fail is true when "func" returns TRUE. */ if ((res= func(thd, plugins[idx], arg))) + { + if (reap_on_fail) + plugin_ref_to_int(plugins[idx])->state= PLUGIN_IS_DELETED;

may be you don't need a new argument for plugin_foreach_with_mask, and can put this logic into the `func` callback? That is into static my_bool signal_ddl_recovery_done(THD *, plugin_ref plugin, void *) { handlerton *hton= plugin_hton(plugin); if (hton->signal_ddl_recovery_done) if ((hton->signal_ddl_recovery_done)(hton)) plugin_ref_to_int(plugin)->state= PLUGIN_IS_DELETED; return 0; } or something like that.

+ else break; + } }

plugin_unlock_list(0, plugins, total); diff --git a/storage/spider/spd_table.cc b/storage/spider/spd_table.cc index a9782a8a822..e50942d449f 100644 --- a/storage/spider/spd_table.cc +++ b/storage/spider/spd_table.cc @@ -6376,15 +6376,13 @@ bool spider_init_system_tables() DBUG_RETURN(TRUE); }

- const int size= sizeof(spider_init_queries) / sizeof(spider_init_queries[0]); - for (int i= 0; i < size; i++) + for (uint i= 0; i < size; i++) { - const LEX_STRING *query= &spider_init_queries[i]; + const LEX_STRING *query= &queries[i];

I don't see much of a point, why not to run *all* queries from spider_ddl_recovery_done() ?

if (mysql_real_query(mysql, query->str, query->length)) { - fprintf(stderr, - "[ERROR] SPIDER plugin initialization failed at '%s' by '%s'\n", - query->str, mysql_error(mysql)); + sql_print_error("SPIDER: plugin initialization failed at '%s' by '%s'\n", + query->str, mysql_error(mysql));

mysql_close(mysql); DBUG_RETURN(TRUE); @@ -6667,10 +6665,20 @@ int spider_db_init( spider_udf_table_mon_list_hash[roop_count].array.size_of_element); }

- if (spider_init_system_tables()) - { + if (spider_init_system_tables( + spider_init_queries, + sizeof(spider_init_queries) / sizeof(spider_init_queries[0]))) + goto error_system_table_creation; + + /* If plugin_init() has already been called, it implies this + function is called after the ddl recovery is done, thus we can + safely execute ALTER TABLE init queries. Otherwise it is deferred to + the spider_ddl_recovery_done() callback. */ + if (plugins_are_initialized && + spider_init_system_tables(spider_init_alter_table_queries, + sizeof(spider_init_alter_table_queries) / + sizeof(spider_init_alter_table_queries[0])))

Don't do that, instead, please, take the "ddl_recovery_done" change from the commit 0930eb86cb00edb2a - it makes the storage engine API simpler, the engine doesn't need to implement two code paths and try to detect whether it's loaded at run-time or at startup.

goto error_system_table_creation; - }

if (!(spider_table_sts_threads = (SPIDER_THREAD *) spider_bulk_malloc(NULL, 256, MYF(MY_WME | MY_ZEROFILL),

Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org