Re: [Maria-developers] 1581f65be57: MDEV-13301 Optimize DROP INDEX, ADD INDEX into RENAME INDEX
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
Hi, Sergei. Thank you for the review. Style fixed. 01.04.2019, 17:18, "Sergei Golubchik" <serg@mariadb.org>:
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?
Well, it's a different issue MDEV-7318 which is not assigned to me. I wasn't initiative enough to implement it too. I may do it if you want but I would like to do something related to InnoDB instead.
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.
Thanks for noticing that. Equal indexes already not added to `index_add_buffer` and `index_drop_buffer` so I replaced my condition with an assertion.
+ + 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
-- Eugene
Hi, Eugene! On Apr 02, Eugene Kosov wrote:
Hi, Sergei.
Thank you for the review. Style fixed.
01.04.2019, 17:18, "Sergei Golubchik" <serg@mariadb.org>:
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?
Well, it's a different issue MDEV-7318 which is not assigned to me. I wasn't initiative enough to implement it too. I may do it if you want but I would like to do something related to InnoDB instead.
Okay. With the latest style fixes this PR looks fine. But, please, rebase it on top of the latest 10.4. It causes numerous merge conflicts with your "remove dead code" commit. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergei! 02.04.2019, 20:55, "Sergei Golubchik" <serg@mariadb.org>:
Hi, Eugene!
On Apr 02, Eugene Kosov wrote:
Hi, Sergei.
Thank you for the review. Style fixed.
01.04.2019, 17:18, "Sergei Golubchik" <serg@mariadb.org>: > 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?
Well, it's a different issue MDEV-7318 which is not assigned to me. I wasn't initiative enough to implement it too. I may do it if you want but I would like to do something related to InnoDB instead.
Okay. With the latest style fixes this PR looks fine.
But, please, rebase it on top of the latest 10.4. It causes numerous merge conflicts with your "remove dead code" commit.
Done. Sorry I didn't noticed that for myself.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
-- Eugene
participants (2)
-
Eugene Kosov -
Sergei Golubchik