maria-developers team mailing list archive

[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?
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.

> +
> +      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