[#MDEV-32559] Move alter table statements in spider init queries to be executed in the signal_ddl_recovery

Comment by Yuchen Pei [ 2023-10-24 ]

One concern with doing this is the breaking of the assumption that plugin_init() does and completes the plugin initialisation. There are about 280 lines between the call to plugin_init() and the call to ha_signal_ddl_recovery_done();. If any of these lines does things based on the assumption that the plugin initialisation is finished then things could break:

static int init_server_components()

//  [... 324 lines elided]

  if (!opt_abort && ddl_log_initialize())

    unireg_abort(1);

  if (plugin_init(&remaining_argc, remaining_argv,

                  (opt_noacl ? PLUGIN_INIT_SKIP_PLUGIN_TABLE : 0) |

                  (opt_abort ? PLUGIN_INIT_SKIP_INITIALIZATION : 0)))

    sql_print_error("Failed to initialize plugins.");

    unireg_abort(1);

//  [... 281 lines elided]

  if (ddl_log_execute_recovery() > 0)

    unireg_abort(1);

  ha_signal_ddl_recovery_done();

//  [... 46 lines elided]

monty, marko: Thoughts?

Comment by Yuchen Pei [ 2023-11-03 ]

A patch ready for review:

upstream/bb-10.10-mdev-32559 5112f67ce87a91820f0f1bf28d441c5bd60b1a9b

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.

I also have a patch demonstrating the failure path resulting in plugin
deinit. I'm not sure how to incorporate it into an mtr case though:

upstream/bb-10.10-mdev-32559-failure-demo f1b7758cb91bdf3ab003b77f8fbe8d19b418163e

MDEV-32559 [demo] Add a bad spider init query to show failure in ddl_recovery_done causes deinit of the plugin

See the test spider/bugfix.plugin_load_add_all

Comment by Yuchen Pei [ 2023-11-08 ]

Hi serg, ptal thanks

[Revision 014167880d2af3cb49413303a2cc1074552f75c9]

Author: Yuchen Pei <ycp@mariadb.com>

Date: 2023-11-08 Wed 13:10:38 AEDT

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.

Comment by Yuchen Pei [ 2024-01-10 ]

serg thanks for the review. Given your change 0930eb86cb00edb2ae285c3250ca51bae126e5e5 with overlapping purposes was applied to 10.6, I have rebased my patch to 10.6. PTAL thanks

bb-10.6-mdev-32559 7077d94f491a0ddd644a296db69489e4fd9ad292

MDEV-32559 failing spider signal_ddl_recovery_done callback should result in spider deinit

Since 0930eb86cb00edb2ae285c3250ca51bae126e5e5, system table creation

needed for spider init is delayed to the signal_ddl_recovery_done

callback. Since it is part of the init, failure should result in

spider deinit.

We also remove the call to spider_init_system_tables() from

spider_db_init(), as it was removed in the commit mentioned above and

accidentally restored in a merge.

Comment by Sergei Golubchik [ 2024-01-11 ]

in 7077d94f491a, please, reformat comments to match the comment style for the file you're changing. Then ok to push

Comment by Yuchen Pei [ 2024-01-16 ]

Thanks for the review. After a quick grep still not entirely sure how a multiline comment should appear for int (*init)(void *); as the other fields all have short comments to the right. So I merged the comment to the right of this field into the comment above.

Comment by Yuchen Pei [ 2024-01-18 ]

Pushed 931df937e9382248ab082e4b3abd8ed149d48cd4 to 10.6 (3 days ago)

[MDEV-32559] Move alter table statements in spider init queries to be executed in the signal_ddl_recovery_done callback Created: 2023-10-24 Updated: 2024-01-18 Resolved: 2024-01-18
Status:	Closed
Project:	MariaDB Server
Component/s:	Storage Engine - Spider
Fix Version/s:	10.6.17

[MDEV-32559] Move alter table statements in spider init queries to be executed in the signal_ddl_recovery_done callback Created: 2023-10-24 Updated: 2024-01-18 Resolved: 2024-01-18