[MDEV-21832] FORCE all partition to rebuild if any one of the partition does rebuild Created: 2020-02-27  Updated: 2020-05-05  Resolved: 2020-03-30

Status: Closed
Project: MariaDB Server
Component/s: Partitioning, Storage Engine - InnoDB
Affects Version/s: 10.3, 10.4, 10.5
Fix Version/s: 10.3.23, 10.4.13, 10.5.3

Type: Bug Priority: Major
Reporter: Thirunarayanan Balathandayuthapani Assignee: Thirunarayanan Balathandayuthapani
Resolution: Fixed Votes: 0
Labels: None

Attachments: File workaround_MDEV-21832.patch    
Issue Links:
Relates
relates to MDEV-22465 DROP COLUMN, DROP INDEX is wrongly cl... Closed

 Description   

During DDL, if one partition requires table rebuild and other partition doesn't need rebuild then all partition should be forced to rebuild.

In ha_innobase::commit_inplace_alter_table() assumes that all partition does same operation

       /* Apply the changes to the data dictionary tables, for all
        partitions. */
 
        for (inplace_alter_handler_ctx** pctx = ctx_array;
             *pctx && !fail; pctx++) {
                ha_innobase_inplace_ctx*        ctx
                        = static_cast<ha_innobase_inplace_ctx*>(*pctx);
 
                DBUG_ASSERT(new_clustered == ctx->need_rebuild());
 
                fail = commit_set_autoinc(ha_alter_info, ctx, altered_table,
                                          table);



 Comments   
Comment by Thirunarayanan Balathandayuthapani [ 2020-02-28 ]

Patch is in bb-10.4-MDEV-21832

Comment by Sergei Golubchik [ 2020-03-02 ]

Why would ha_partition need to rebuild all partitions?
Because InnoDB assumes that? Why InnoDB does it?

Comment by Marko Mäkelä [ 2020-03-02 ]

serg, since MySQL 5.6.10 (the first GA version), ha_innobase::commit_inplace_alter_table() does the internal commit for all partitions, so that the ALTER TABLE operation is atomic. That code is assuming that all partitions are handled in the same way (rebuild or not).

It might be possible, but much trickier, to allow a subset of the partitions to be rebuilt.

Comment by Sergei Golubchik [ 2020-03-02 ]

Yes, but ha_partition is an upper-layer storage engine. It should not be tweaked to whatever limitations individual engines might have.

If InnoDB wants all partitions to be rebuilt, it should try to handle this case internally, if possible. Without contaminating ha_partition with InnoDB-specific changes.

Comment by Thirunarayanan Balathandayuthapani [ 2020-03-10 ]

Just adding the workaround patch for future reference workaround_MDEV-21832.patch

Comment by Sergei Golubchik [ 2020-03-26 ]

here's fix I suggest

diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
index 0434b891ffc..dfd622e0206 100644
--- a/sql/ha_partition.cc
+++ b/sql/ha_partition.cc
@@ -10099,7 +10099,8 @@ ha_partition::check_if_supported_inplace_alter(TABLE *altered_table,
                                                Alter_inplace_info *ha_alter_info)
 {
   uint index= 0;
-  enum_alter_inplace_result result= HA_ALTER_INPLACE_NO_LOCK;
+  enum_alter_inplace_result result;
+  alter_table_operations orig_ops;
   ha_partition_inplace_ctx *part_inplace_ctx;
   bool first_is_set= false;
   THD *thd= ha_thd();
@@ -10126,6 +10127,10 @@ ha_partition::check_if_supported_inplace_alter(TABLE *altered_table,
   if (!part_inplace_ctx->handler_ctx_array)
     DBUG_RETURN(HA_ALTER_ERROR);
 
+  do {
+    orig_ops= ha_alter_info->handler_flags;
+    result= HA_ALTER_INPLACE_NO_LOCK;
+
   /* Set all to NULL, including the terminating one. */
   for (index= 0; index <= m_tot_parts; index++)
     part_inplace_ctx->handler_ctx_array[index]= NULL;
@@ -10154,6 +10159,8 @@ ha_partition::check_if_supported_inplace_alter(TABLE *altered_table,
       break;
   }
 
+  } while (orig_ops != ha_alter_info->handler_flags);
+
   ha_alter_info->handler_ctx= part_inplace_ctx;
   /*
     To indicate for future inplace calls that there are several

this is the partitioning part. The logic here — if ha_alter_info->handler_flags changes, we need to ask all partitions again, as it's basically a different ALTER TABLE now.

And InnoDB, inside ha_innobase::check_if_supported_inplace_alter can set ALTER_RECREATE when needed.

Comment by Thirunarayanan Balathandayuthapani [ 2020-03-27 ]

Patch is in bb-10.3-MDEV-21832 and bb-10.4-MDEV-21832

Comment by Marko Mäkelä [ 2020-03-30 ]

OK to push to 10.3.

Generated at Thu Feb 08 09:10:09 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.