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.