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 <clap...@yandex.ru>
> committer: Eugene Kosov <clap...@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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
