maria-developers team mailing list archive

[Eugene Kosov <claprix@xxxxxxxxx>]

Hi, Sergei.

Thank you for the review. Style fixed.

01.04.2019, 17:18, "Sergei Golubchik" <serg@xxxxxxxxxxx>:
> 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@xxxxxxxxx>
>>  committer: Eugene Kosov <claprix@xxxxxxxxx>
>>  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@xxxxxxxxxxx

-- 
Eugene