maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey!

On Oct 29, Aleksey Midenkov wrote:
> revision-id: e752eed5354 (mariadb-10.4.7-47-ge752eed5354)
> parent(s): 8403148452a
> author: Aleksey Midenkov <midenok@xxxxxxxxx>
> committer: Aleksey Midenkov <midenok@xxxxxxxxx>
> timestamp: 2019-08-19 12:00:13 +0300
> message:
> 
> MDEV-19751 Wrong partitioning by KEY() after key dropped
> 
> Empty partition by key() clause implies that key might be changed by
> ALTER, but inplace algorithm must not be used since repartitioning is
> required for a different key.

good and very clear comment.
 
> diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc
> index 10b9c74d868..d76ff5e2e76 100644
> --- a/sql/sql_partition.cc
> +++ b/sql/sql_partition.cc
> @@ -5903,6 +5903,45 @@ the generated partition syntax in a correct manner.
>            *partition_changed= TRUE;
>          }
>        }
> +      /*
> +        Prohibit inplace when key takes part in partitioning expression
> +        and is altered (dropped).
> +      */
> +      if (!*partition_changed && tab_part_info->part_field_array)
> +      {
> +        KEY *key_info= table->key_info;
> +        List_iterator_fast<Alter_drop> drop_it(alter_info->drop_list);
> +        for (uint key= 0; key < table->s->keys; key++, key_info++)
> +        {
> +          if (key_info->flags & HA_INVISIBLE_KEY)
> +            continue;
> +          const char *key_name= key_info->name.str;
> +          const Alter_drop *drop;
> +          drop_it.rewind();
> +          while ((drop= drop_it++))
> +          {
> +            if (drop->type == Alter_drop::KEY &&
> +                0 == my_strcasecmp(system_charset_info, key_name, drop->name))
> +              break;
> +          }
> +          if (!drop)
> +            continue;
> +          for (uint kp= 0; kp < key_info->user_defined_key_parts; ++kp)
> +          {
> +            const KEY_PART_INFO &key_part= key_info->key_part[kp];
> +            for (Field **part_field= tab_part_info->part_field_array;
> +                *part_field; ++part_field)
> +            {
> +              if (*part_field == key_part.field)
> +              {
> +                *partition_changed= TRUE;
> +                goto search_finished;
> +              }
> +            } // for (part_field)
> +          } // for (key_part)
> +        } // for (key_info)

Hmm, if I read it correctly, you iterate over all key parts of every
dropped key and see if this field is also part of the partitioning
expression.

This sounds kind of strange. What if there are two indexes that use the
column `a`, say

  CREATE TABLE t1 (a int, b int, index(a), index(b,a))

and you partition by key(a). Dropping the second index does not change
the partitioning, does it?

On the other hand, if it's ALTER TABLE ... MODIFY `a` then it can change
partitioning, even when the key hasn't changed, right?

So, I suspect your check is too strong in some cases and too loose is
others.

> +        search_finished:;
> +      }
>      }
>      if (thd->work_part_info)
>      {
> diff --git a/storage/connect/mysql-test/connect/r/part_file.result b/storage/connect/mysql-test/connect/r/part_file.result
> index 3dabd946b50..480de51cce6 100644
> --- a/storage/connect/mysql-test/connect/r/part_file.result
> +++ b/storage/connect/mysql-test/connect/r/part_file.result
> @@ -308,12 +308,19 @@ EXPLAIN PARTITIONS SELECT * FROM t1 WHERE id = 10;
>  id	select_type	table	partitions	type	possible_keys	key	key_len	ref	rows	Extra
>  1	SIMPLE	t1	1	ref	XID	XID	4	const	1	
>  DROP INDEX XID ON t1;
> +Warnings:
> +Warning	1105	Data repartition in 1 is unchecked
> +Warning	1105	Data repartition in 2 is unchecked
> +Warning	1105	Data repartition in 3 is unchecked

why did the result file change?
could you add a short explanation of it to the commit comment, please?

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx