Hi, Eugene! See the review below. This looked pretty much ok, a couple of style comments. And why did you not implement the new ALTER TABLE .. .RENAME INDEX syntax? I agree that a pair of DROP/ADD should act as RENAME. But a dedicated RENAME would be also good to have, at least for compatibility reasons. On Apr 01, Eugene Kosov wrote:
revision-id: 1581f65be57 (mariadb-10.4.3-69-g1581f65be57) parent(s): e012d266807 author: Eugene Kosov <claprix@yandex.ru> committer: Eugene Kosov <claprix@yandex.ru> timestamp: 2019-03-15 18:44:05 +0300 message:
MDEV-13301 Optimize DROP INDEX, ADD INDEX into RENAME INDEX
Just rename index in data dictionary and in InnoDB cache when it's possible. Introduce ALTER_INDEX_RENAME for that purpose so that engines can optimize such operation.
Unused code between macro MYSQL_RENAME_INDEX was removed.
compare_keys_but_name(): compare index definitions except for index names
Alter_inplace_info::rename_keys: ha_innobase_inplace_ctx::rename_keys: vector of rename indexes
fill_alter_inplace_info():: fills Alter_inplace_info::rename_keys
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index e1337a0f710..e1131422117 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -7062,6 +7061,43 @@ static bool fill_alter_inplace_info(THD *thd, TABLE *table, bool varchar, new_key->option_struct; }
+ uint &add_count= ha_alter_info->index_add_count;
Coding style: no modifiable references please. write ha_alter_info->index_add_count++ same below
+ for (uint i= 0; i < add_count; i++) + { + uint *&add_buffer= ha_alter_info->index_add_buffer; + const KEY *new_key= ha_alter_info->key_info_buffer + add_buffer[i]; + + uint &drop_count= ha_alter_info->index_drop_count; + for (uint j= 0; j < drop_count; j++) + { + KEY **&drop_buffer= ha_alter_info->index_drop_buffer; + const KEY *old_key= drop_buffer[j]; + + if (!lex_string_cmp(system_charset_info, &old_key->name, &new_key->name)) + continue;
Why? I'd think and if it's Compare_keys::Equal and the name is the same too, you can simply remove add/drop elements without adding a rename element.
+ + if (Compare_keys::Equal != compare_keys_but_name(old_key, new_key, + alter_info, table, + new_pk, old_pk))
Coding style: don't invert comparisons
+ { + continue; + } + + ha_alter_info->handler_flags|= ALTER_RENAME_INDEX; + ha_alter_info->rename_keys.push_back( + Alter_inplace_info::Rename_key_pair(old_key, new_key)); + + memcpy(add_buffer + i, add_buffer + i + 1, + sizeof(add_buffer[0]) * (add_count - i - 1)); + memcpy(drop_buffer + j, drop_buffer + j + 1, + sizeof(drop_buffer[0]) * (drop_count - j - 1)); + --add_count; + --drop_count; + --i; // this index once again + break; + } + } +
Regards, Sergei Chief Architect MariaDB and security@mariadb.org