database/mariadb
1
0
Fork 0
mirror of https://github.com/MariaDB/server.git synced 2025-07-29 05:21:33 +03:00
Code Activity

MDEV-30984 Online ALTER table is denied with non-informative error messages

Browse Source 
Group all the checks in online_alter_check_supported().

There is now two groups of checks:
1. A technical availability of online, that is checked before open_tables,
and affects table_list->lock_type. It's supposed to be safe to make it
TL_READ even if COPY algorithm will fall back to not-online, since MDL is
SHARED_UPGRADEABLE anyway.
2. An 'online' availability for a COPY algorithm. It can be done as late as
just before the copy_data_between_tables call. The lock_type influence is
disclosed above, so the only other place it affects is
Alter_info::supports_lock, where `online` flag is only used to decide
whether to report the error at the inplace preparation stage. We'd want to
make that at the last resort, which is COPY preparation, if no algorithm is
chosen by the user. So it's event better now.

Some changes are required to the autoinc support detection, as the check
now happens after mysql_prepare_alter_table:
* alter_info->drop_list is empty
* instead, dropped columns are in tmp_set
* alter_info->create_list now has every field that's in the new table.
* the column definition's change.str will be nonnull whether the column
  remains in the new table (vs whether it was changed, as before).
  But it also has `field` field set.
* IF EXISTS doesn't have to be dealt anymore

This infers that the changes are now checked in more detail: a field's
definition shouldn't be changed, vs a field shouldn't be mentioned in
the CHANGE list, as it was before. This is reflected by the line 193 test.
This commit is contained in:
Nikita Malyavin
2023-05-15 19:39:21 +03:00
committed by Sergei Golubchik
parent b3f988d260
commit c382de72ea
5 changed files with 105 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

123
sql/sql_table.cc
View File

@ -9871,26 +9871,21 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info,
*/
for (uint k= 0; k < table->s->keys; k++)
{
const KEY &key= table->s->key_info[k];
const KEY &key= table->key_info[k];
if ((key.flags & HA_NOSAME) == 0 || key.flags & HA_NULL_PART_KEY)
continue;
bool key_parts_good= true;
for (uint kp= 0; kp < key.user_defined_key_parts && key_parts_good; kp++)
{
const char *field_name= key.key_part[kp].field->field_name.str;
for (const auto &c: alter_info->drop_list)
if (c.type == Alter_drop::COLUMN
&& my_strcasecmp(system_charset_info, c.name, field_name) == 0)
{
key_parts_good= false;
break;
}
const Field *f= key.key_part[kp].field;
// tmp_set contains dropped fields after mysql_prepare_alter_table
key_parts_good= !bitmap_is_set(&table->tmp_set, f->field_index);
if (key_parts_good)
for (const auto &c: alter_info->create_list)
if (c.change.str && my_strcasecmp(system_charset_info, c.change.str,
field_name) == 0)
if (c.field == f)
{
key_parts_good= false;
key_parts_good= f->is_equal(c);
break;
}
}
@ -9898,25 +9893,67 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info,
return true;
}
const char *old_autoinc= table->found_next_number_field
? table->found_next_number_field->field_name.str
: "";
bool online= true;
for (const auto &c: alter_info->create_list)
{
if (c.change.str && c.flags & AUTO_INCREMENT_FLAG)
if (c.flags & AUTO_INCREMENT_FLAG)
{
if (my_strcasecmp(system_charset_info, c.change.str, old_autoinc) != 0)
{
if (c.create_if_not_exists // check IF EXISTS option
&& table->find_field_by_name(&c.change) == NULL)
continue;
online= false;
}
if (c.field && !(c.field->flags & AUTO_INCREMENT_FLAG))
return false;
break;
}
}
return online;
return true;
}
static
const char *online_alter_check_supported(const THD *thd,
const Alter_info *alter_info,
const TABLE *table, bool *online)
{
DBUG_ASSERT(*online);
*online= thd->locked_tables_mode != LTM_LOCK_TABLES && !table->s->tmp_table;
if (!*online)
return NULL;
*online= (alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) == 0;
if (!*online)
return "DROP SYSTEM VERSIONING";
*online= !thd->lex->ignore;
if (!*online)
return "ALTER IGNORE TABLE";
*online= !table->versioned(VERS_TRX_ID);
if (!*online)
return "BIGINT GENERATED ALWAYS AS ROW_START";
List<FOREIGN_KEY_INFO> fk_list;
table->file->get_foreign_key_list(thd, &fk_list);
for (auto &fk: fk_list)
{
if (fk_modifies_child(fk.delete_method) ||
fk_modifies_child(fk.update_method))
{
*online= false;
// Don't fall to a common unsupported case to avoid heavy string ops.
if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE)
{
return fk_modifies_child(fk.delete_method)
? thd->strcat({STRING_WITH_LEN("ON DELETE ")},
*fk_option_name(fk.delete_method)).str
: thd->strcat({STRING_WITH_LEN("ON UPDATE ")},
*fk_option_name(fk.update_method)).str;
}
return NULL;
}
}
*online= online_alter_check_autoinc(thd, alter_info, table);
if (!*online)
return "CHANGE COLUMN ... AUTO_INCREMENT";
return NULL;
}
@ -10100,9 +10137,7 @@ bool mysql_alter_table(THD *thd, const LEX_CSTRING *new_db,
table_list->required_type= TABLE_TYPE_NORMAL;
if (alter_info->requested_lock > Alter_info::ALTER_TABLE_LOCK_NONE
|| alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING
|| thd->lex->sql_command == SQLCOM_OPTIMIZE
|| ignore
|| alter_info->algorithm(thd) > Alter_info::ALTER_TABLE_ALGORITHM_COPY)
online= false;
@ -10142,22 +10177,6 @@ bool mysql_alter_table(THD *thd, const LEX_CSTRING *new_db,
table= table_list->table;
bool is_reg_table= table->s->tmp_table == NO_TMP_TABLE;
online= online && !table->s->tmp_table && !table->versioned(VERS_TRX_ID);
List<FOREIGN_KEY_INFO> fk_list;
table->file->get_foreign_key_list(thd, &fk_list);
for (auto &fk: fk_list)
{
if (fk_modifies_child(fk.delete_method)
|| fk_modifies_child(fk.update_method))
{
online= false;
break;
}
}
online= online && online_alter_check_autoinc(thd, alter_info, table);
#ifdef WITH_WSREP
if (WSREP(thd) &&
(thd->lex->sql_command == SQLCOM_ALTER_TABLE ||
@ -10979,22 +10998,22 @@ do_continue:;
if (fk_prepare_copy_alter_table(thd, table, alter_info, &alter_ctx))
goto err_new_table_cleanup;
if (!table->s->tmp_table)
if (online)
{
// COPY algorithm doesn't work with concurrent writes.
if (!online &&
const char *reason= online_alter_check_supported(thd, alter_info, table,
&online);
if (reason &&
alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE)
{
DBUG_ASSERT(!online);
my_error(ER_ALTER_OPERATION_NOT_SUPPORTED_REASON, MYF(0),
"LOCK=NONE",
ER_THD(thd, ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY),
"LOCK=SHARED");
"LOCK=NONE", reason, "LOCK=SHARED");
goto err_new_table_cleanup;
}
}
if (thd->locked_tables_mode == LTM_LOCK_TABLES)
online= false;
if (!table->s->tmp_table)
{
// If EXCLUSIVE lock is requested, upgrade already.
if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_EXCLUSIVE &&
wait_while_table_is_used(thd, table, HA_EXTRA_FORCE_REOPEN))