Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-32559

Move alter table statements in spider init queries to be executed in the signal_ddl_recovery_done callback

Details

    Description

      According to monty's comment on MDEV-32532[1], and related discussions with marko. No ALTER TABLE statements should be executed before recovery is done.

      [1] https://jira.mariadb.org/browse/MDEV-32532?focusedCommentId=272499&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-272499

      This task is more relevant after the push of MDEV-22979, because spider executes init queries synchoronously afterwards, and we can no longer "count on" race conditions for the ddl to happen after the recovery... As such, we mark this ticket as caused by that ticket, so that we don't forget to do the same for MDEV-29870.

      Attachments

        Issue Links

          Activity

            ycp Yuchen Pei added a comment - - edited

            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?

            ycp Yuchen Pei added a comment - - edited 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?
            ycp Yuchen Pei added a comment -

            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

            ycp Yuchen Pei added a comment - 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
            ycp Yuchen Pei added a comment - - edited

            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.
            

            ycp Yuchen Pei added a comment - - edited 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.
            ycp Yuchen Pei added a comment -

            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.
            

            ycp Yuchen Pei added a comment - 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.

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

            serg Sergei Golubchik added a comment - in 7077d94f491a, please, reformat comments to match the comment style for the file you're changing. Then ok to push
            ycp Yuchen Pei added a comment -

            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.

            ycp Yuchen Pei added a comment - 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.
            ycp Yuchen Pei added a comment -

            Pushed 931df937e9382248ab082e4b3abd8ed149d48cd4 to 10.6 (3 days ago)

            ycp Yuchen Pei added a comment - Pushed 931df937e9382248ab082e4b3abd8ed149d48cd4 to 10.6 (3 days ago)

            People

              ycp Yuchen Pei
              ycp Yuchen Pei
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.