Hi, Sergei. Thank you for the review. Style fixed. 01.04.2019, 17:18, "Sergei Golubchik" :

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  committer: Eugene Kosov  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