Details
-
Bug
-
Status: Closed (View Workflow)
-
Critical
-
Resolution: Fixed
-
10.5, 10.6
Description
If a table have partitions and you try to change a column
(modify or change) with an index, which is not the partitions key,
algorithm = instant not work.
MariaDB [l1]> ALTER online TABLE t1 MODIFY COLUMN `f4` VARCHAR(500) , ALGORITHM=INSTANT, LOCK=NONE; |
ERROR 1846 (0A000): ALGORITHM=INSTANT is not supported. Reason: ADD INDEX. Try ALGORITHM=NOCOPY |
To reproduce:
|
|
|
|
drop table if exists t1; |
|
|
|
|
CREATE TABLE `t1` (
|
`f1` datetime ,
|
`f2` VARCHAR(2) , |
`f3` VARCHAR(200) , |
`f4` VARCHAR(100) , |
INDEX `i3` (`f4`) )
|
/*!50100 PARTITION BY RANGE COLUMNS(`f2`) (PARTITION `p_01` VALUES LESS THAN ('02') ENGINE = InnoDB, PARTITION `p_31` VALUES LESS THAN (MAXVALUE) ENGINE = InnoDB) */; |
|
|
|
|
ALTER online TABLE t1 MODIFY COLUMN `f4` VARCHAR(500) , ALGORITHM=INSTANT, LOCK=NONE; |
If you do the testcase without index
OR !
without partition term,
it works.
Attachments
Issue Links
- relates to
-
-
- Stalled
-
-
-
- Closed
-
Activity
Thanks for the analysis, thiru.
So in mysql_alter_table(), it first determine ha_alter_info->handler_flags in fill_alter_inplace_info(), then assign to ha_alter_info.inplace_supported with the value of table->file->check_if_supported_inplace_alter(), and finally determines whether the specified algorithm is supported in alter_info->supports_algorithm.
// mysql_alter_table:
|
if (fill_alter_inplace_info(thd, table, varchar, &ha_alter_info)) |
goto err_new_table_cleanup; |
// [... 80 lines elided]
|
ha_alter_info.inplace_supported=
|
table->file->check_if_supported_inplace_alter(&altered_table,
|
&ha_alter_info);
|
// [... 15 lines elided]
|
if (alter_info->supports_algorithm(thd, &ha_alter_info) || |
In fill_alter_inplace_info(), if compare_keys_but_name() returns Compare_keys::NotEqual the handler_flags will have ALTER_ADD_NON_UNIQUE_NON_PRIM_INDEX:
static bool fill_alter_inplace_info(THD *thd, TABLE *table, bool varchar, |
Alter_inplace_info *ha_alter_info)
|
{
|
// [... 341 lines elided]
|
switch (compare_keys_but_name(table_key, new_key, alter_info, table, new_pk, |
old_pk))
|
{
|
case Compare_keys::Equal: |
continue; |
case Compare_keys::EqualButKeyPartLength: |
ha_alter_info->handler_flags|= ALTER_COLUMN_INDEX_LENGTH;
|
continue; |
case Compare_keys::EqualButComment: |
ha_alter_info->handler_flags|= ALTER_CHANGE_INDEX_COMMENT;
|
continue; |
case Compare_keys::NotEqual: // (1) |
break; |
}
|
// [... 5 lines elided]
|
ha_alter_info->index_add_buffer
|
[ha_alter_info->index_add_count++]= // (2) |
// [... 3 lines elided]
|
}
|
// [... 117 lines elided]
|
/* Now figure out what kind of indexes we are adding. */ |
for (uint add_key_idx= 0; add_key_idx < ha_alter_info->index_add_count; add_key_idx++) |
{
|
// [... 9 lines elided]
|
else |
ha_alter_info->handler_flags|= ALTER_ADD_NON_UNIQUE_NON_PRIM_INDEX; // (3) |
}
|
// [... 3 lines elided]
|
}
|
In ha_innobase::check_if_supported_inplace_alter(), having ALTER_ADD_NON_UNIQUE_NON_PRIM_INDEX (part of INNOBASE_ONLINE_CREATE) causes instant_alter_column_possible() to return false, which leads to itself returning HA_ALTER_INPLACE_NOCOPY_NO_LOCK, rather than HA_ALTER_INPLACE_INSTANT if the table engine of t1 was innodb:
// ha_innobase::check_if_supported_inplace_alter:
|
const bool supports_instant = instant_alter_column_possible( |
,*m_prebuilt->table, ha_alter_info, table, altered_table,
|
is_innodb_strict_mode());
|
// [... 38 lines elided]
|
if (supports_instant && !(ha_alter_info->handler_flags |
& INNOBASE_ALTER_NOREBUILD)) {
|
DBUG_RETURN(HA_ALTER_INPLACE_INSTANT);
|
}
|
|
|
// instant_alter_column_possible:
|
if (ha_alter_info->handler_flags |
& ((INNOBASE_ALTER_REBUILD | INNOBASE_ONLINE_CREATE)
|
& ~ALTER_DROP_STORED_COLUMN
|
& ~ALTER_STORED_COLUMN_ORDER
|
& ~ALTER_ADD_STORED_BASE_COLUMN
|
& ~ALTER_COLUMN_NULLABLE
|
& ~ALTER_OPTIONS)) {
|
return false; |
Finally, HA_ALTER_INPLACE_NOCOPY_NO_LOCK results in the error in alter_info->supports_algorithm().
From this analysis, the three steps look rather independent of each other, and it seems the fix could be just calling compare_keys_parts() on the underlying engines, and until MDEV-22168 is done, they are all the same engine, so we do not have to worry about check_if_supported_inplace_alter() called on different engines, and can simply take the maximum return value from all the compare_key_parts() calls, though they are probably all the same value.
Draft patch:
955d60e77a5 bb-10.6-mdev-34813 MDEV-34813 A simple implementation of ha_partition::compare_key_parts
|
The issue is present in 10.5 too, why is the fixversion 10.6? julien.fritsch
A relevant question is: is this a bug or a feature request?
Hi holyfoot, ptal thanks:
7abca6e3b31 upstream/bb-10.5-mdev-34813 MDEV-34813 A simple implementation of ha_partition::compare_key_parts
|
Thanks for the review - pushed 1f306d395d00df158702d35b3338ccfe8663744e to 10.5
CREATE TABLE `t1` (`f1` datetime ,`f2` VARCHAR(2) ,`f3` VARCHAR(200) ,`f4` VARCHAR(100) ,INDEX `i3` (`f4`) )engine=innodb;ALTER online TABLE t1 MODIFY COLUMN `f4` VARCHAR(500) , ALGORITHM=INSTANT, LOCK=NONE;Above test case passes with INSTANT algorithm. Why it passes?
compare_keys_but_name does call compare_key_parts(). Since it is a InnoDB engine,
it does call ha_innobase::compare_key_parts() and returns Compare_keys::EqualButKeyPartLength.
This makes the index not to rebuild.
But in case of partition, alter call handler::compare_key_parts() and returns Compare_keys::NotEqual
This lead to index rebuild. Yes, InnoDB instant doesn't support when InnoDB has to modify the column
and rebuild the index. imo, partition has to implement compare_key_parts() in their code and it should
call underlined engine compare_key_parts().