Hi, Aleksey! On Oct 29, Aleksey Midenkov wrote:

revision-id: e752eed5354 (mariadb-10.4.7-47-ge752eed5354) parent(s): 8403148452a author: Aleksey Midenkov committer: Aleksey Midenkov 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 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@mariadb.org