Re: 331430a6a73: MDEV-33658 2/2 Cannot add a foreign key on a table with a matching long UNIQUE
Hi, Nikita, On Jan 01, Nikita Malyavin wrote:
diff --git a/sql/sql_class.cc b/sql/sql_class.cc index de688b4cb31..2ffceb3932b 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -217,6 +218,16 @@ Foreign_key::Foreign_key(const Foreign_key &rhs, MEM_ROOT *mem_root)
bool is_foreign_key_prefix(Key *a, Key *b) { + ha_key_alg a_alg= a->key_create_info.algorithm; + ha_key_alg b_alg= b->key_create_info.algorithm; + + // The real algorithm will be BTREE is none was given by user.
Technically, this is not correct, the algorithm will be whatever the engine wants. For MEMORY engine it'll be HASH. But only InnoDB can do foreign keys now, and for InnoDB th3 default key algorithm is BTREE. Let's reflect it in the comment, like, for example (also fixed a typo "is" -> "if") // The real algorithm in InnoDB will be BTREE if none was given by user
+ a_alg= a_alg == HA_KEY_ALG_UNDEF ? HA_KEY_ALG_BTREE : a_alg; + b_alg= b_alg == HA_KEY_ALG_UNDEF ? HA_KEY_ALG_BTREE : b_alg; + + if (a_alg != b_alg) + return false; + /* Ensure that 'a' is the generated key */ if (a->generated) { diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 3f035defbd3..d7aeb5d7cbf 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -3556,6 +3556,303 @@ key_add_part_check_null(const handler *file, KEY *key_info, }
+static +my_bool key_check_without_overlaps(THD *thd, HA_CREATE_INFO *create_info, + Alter_info *alter_info, + Key &key) +{ + DBUG_ENTER("key_check_without_overlaps"); + if (!key.without_overlaps) + DBUG_RETURN(FALSE); + // append_system_key_parts is already called, so we should check all the + // columns except the last two. + const auto &period_start= create_info->period_info.period.start; + const auto &period_end= create_info->period_info.period.end; + List_iterator<Key_part_spec> part_it_forwarded(key.columns); + List_iterator<Key_part_spec> part_it(key.columns); + part_it_forwarded++; + part_it_forwarded++; + while (part_it_forwarded++) + { + Key_part_spec *key_part= part_it++; + + if (period_start.streq(key_part->field_name) + || period_end.streq(key_part->field_name)) + { + my_error(ER_KEY_CONTAINS_PERIOD_FIELDS, MYF(0), key.name.str, + key_part->field_name.str); + DBUG_RETURN(TRUE); + } + }
It was simpler to have this check inside append_system_key_parts(), why did you move it?
+ + if (key.key_create_info.algorithm == HA_KEY_ALG_HASH || + key.key_create_info.algorithm == HA_KEY_ALG_LONG_HASH) + + { + my_error(ER_KEY_CANT_HAVE_WITHOUT_OVERLAPS, MYF(0), key.name.str); + DBUG_RETURN(TRUE); + } <...> + KEY_CREATE_INFO *key_cinfo= &key.key_create_info; + + if (is_hash_field_needed) + { + if (key_cinfo->algorithm == HA_KEY_ALG_UNDEF) + key_cinfo->algorithm= HA_KEY_ALG_LONG_HASH;
Perhaps you shouldn't set key algorithm in add_hash_field() then? or, rather replace the assignment with an assert?
+ + if (key_cinfo->algorithm != HA_KEY_ALG_HASH && + key_cinfo->algorithm != HA_KEY_ALG_LONG_HASH) + { + my_error(ER_TOO_LONG_KEY, MYF(0), max_key_length); + DBUG_RETURN(TRUE); + } + } + } + } + DBUG_RETURN(FALSE); +} + + /* Preparation for table creation
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
On Wed, 1 Jan 2025 at 18:22, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
On Jan 01, Nikita Malyavin wrote:
diff --git a/sql/sql_class.cc b/sql/sql_class.cc index de688b4cb31..2ffceb3932b 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -217,6 +218,16 @@ Foreign_key::Foreign_key(const Foreign_key &rhs, MEM_ROOT *mem_root)
bool is_foreign_key_prefix(Key *a, Key *b) { + ha_key_alg a_alg= a->key_create_info.algorithm; + ha_key_alg b_alg= b->key_create_info.algorithm; + + // The real algorithm will be BTREE is none was given by user.
Technically, this is not correct, the algorithm will be whatever the engine wants. For MEMORY engine it'll be HASH.
But only InnoDB can do foreign keys now, and for InnoDB th3 default key algorithm is BTREE. Let's reflect it in the comment, like, for example (also fixed a typo "is" -> "if")
// The real algorithm in InnoDB will be BTREE if none was given by user
Thank you, I couldn't reveal it by myself.
+ a_alg= a_alg == HA_KEY_ALG_UNDEF ? HA_KEY_ALG_BTREE : a_alg; + b_alg= b_alg == HA_KEY_ALG_UNDEF ? HA_KEY_ALG_BTREE : b_alg; + + if (a_alg != b_alg) + return false; + /* Ensure that 'a' is the generated key */ if (a->generated) { diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 3f035defbd3..d7aeb5d7cbf 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -3556,6 +3556,303 @@ key_add_part_check_null(const handler *file, KEY *key_info, }
+static +my_bool key_check_without_overlaps(THD *thd, HA_CREATE_INFO *create_info, + Alter_info *alter_info, + Key &key) +{ + DBUG_ENTER("key_check_without_overlaps"); + if (!key.without_overlaps) + DBUG_RETURN(FALSE); + // append_system_key_parts is already called, so we should check all the + // columns except the last two. + const auto &period_start= create_info->period_info.period.start; + const auto &period_end= create_info->period_info.period.end; + List_iterator<Key_part_spec> part_it_forwarded(key.columns); + List_iterator<Key_part_spec> part_it(key.columns); + part_it_forwarded++; + part_it_forwarded++; + while (part_it_forwarded++) + { + Key_part_spec *key_part= part_it++; + + if (period_start.streq(key_part->field_name) + || period_end.streq(key_part->field_name)) + { + my_error(ER_KEY_CONTAINS_PERIOD_FIELDS, MYF(0), key.name.str, + key_part->field_name.str); + DBUG_RETURN(TRUE); + } + }
It was simpler to have this check inside append_system_key_parts(), why did you move it?
It's important to append system key parts before duplicates elimination, but the key names generation may only happen afterwards. And we need a key name to report this error, so append_system_key_parts is not a good place for error reports, as it may miss some details. What could be done is some "early guess a key name" function written, specially for error reports. I decided, that it's fine to make an analysis just a bit later.
+ KEY_CREATE_INFO *key_cinfo= &key.key_create_info; + + if (is_hash_field_needed) + { + if (key_cinfo->algorithm == HA_KEY_ALG_UNDEF) + key_cinfo->algorithm= HA_KEY_ALG_LONG_HASH;
Perhaps you shouldn't set key algorithm in add_hash_field() then? or, rather replace the assignment with an assert?
It's not the only way mysql_prepare_create_table enters add_hash_field. This condition: if (is_hash_field_needed || (key_info->algorithm == HA_KEY_ALG_HASH && key->type != Key::PRIMARY && key_info->flags & HA_NOSAME && !(file->ha_table_flags() & HA_CAN_HASH_KEYS ) && file->ha_table_flags() & HA_CAN_VIRTUAL_COLUMNS)) { Create_field *hash_fld= add_hash_field(thd, &alter_info->create_list, key_info); ... supposes, that various keys with ALGORITHM=HASH may also become logn hash keys, if the engine doesn't support hash-based keys. Though, maybe it can be generalized altogether, and then some other condition may apply to such fields, like avoiding pack flags (see KEY_pack_flags call) I may try to make it in a separate commit. -- Yours truly, Nikita Malyavin
Sergei, I forgot to say hello in the last email 😇 so, hello;) On Thu, 9 Jan 2025 at 21:52, Nikita Malyavin <nikitamalyavin@gmail.com> wrote:
On Wed, 1 Jan 2025 at 18:22, Sergei Golubchik <serg@mariadb.org> wrote:
+ KEY_CREATE_INFO *key_cinfo= &key.key_create_info; + + if (is_hash_field_needed) + { + if (key_cinfo->algorithm == HA_KEY_ALG_UNDEF) + key_cinfo->algorithm= HA_KEY_ALG_LONG_HASH;
Perhaps you shouldn't set key algorithm in add_hash_field() then? or, rather replace the assignment with an assert?
No, it still could be set from HA_KEY_ALG_HASH for some cases. However, I have made some refactoring to is_hash_field_needed. Can you please take a look at it as well?
https://github.com/MariaDB/server/commit/b92c73320ac14b2aa0ad6cb423f37db4622... Thank you! Nikita.
participants (2)
-
Nikita Malyavin -
Sergei Golubchik