Hi, Nikita, On Jun 24, Nikita Malyavin wrote:

revision-id: d23555b1920 (mariadb-11.0.1-138-gd23555b1920) parent(s): df90bf34644 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2023-06-20 16:10:48 +0300 message:

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

diff --git a/mysql-test/main/alter_table_online.result b/mysql-test/main/alter_table_online.result --- a/mysql-test/main/alter_table_online.result +++ b/mysql-test/main/alter_table_online.result @@ -179,25 +179,27 @@ t CREATE TABLE `t` ( UNIQUE KEY `b` (`b`) ) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci alter table t drop b, change c c serial, algorithm=copy, lock=none; -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED # Check existed unique keys. create or replace table t(a int, b int not null, c int not null, d int); # No unique in the old table; alter table t add unique(b, c), modify d int auto_increment, add key(d), algorithm=copy, lock=none; -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED alter table t add unique(a, b); # Unique in the old table has nulls; alter table t modify d int auto_increment, add key(d), algorithm=copy, lock=none; -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED alter table t add unique(b, c); # Change unique'scolumn;

Typo

-alter table t change b x int, modify d int auto_increment, add key(d), +alter table t change b x bigint, modify d int auto_increment, add key(d), algorithm=copy, lock=none; -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED -# Finally good. -alter table t modify d int auto_increment, add key(d), +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED +# Finally good. Simple renames with a type unchenged will not affect

typo s/unchenged/unchanged/

+# the result. Also NOT NULL -> NULL transform is fine. +alter table t modify d int auto_increment, add key(d), +change b x int null, algorithm=copy, lock=none; drop table t; # MDEV-31172 Server crash or ASAN errors in online_alter_check_autoinc diff --git a/sql/sql_class.h b/sql/sql_class.h --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1228,6 +1228,15 @@ class Query_arena { return strdup_root(mem_root,str); } inline char *strmake(const char *str, size_t size) const { return strmake_root(mem_root,str,size); } + inline LEX_CSTRING strcat(const LEX_CSTRING &a, const LEX_CSTRING &b) const + { + char *buf= (char*)alloc(a.length + b.length + 1); + if (unlikely(!buf)) + return null_clex_str; + strncpy(buf, a.str, a.length);

memcpy both ↑ and ↓ (and then you might need to append \0 explicitly)

+ strncpy(buf + a.length, b.str, b.length + 1); + return {buf, a.length + b.length}; + } inline void *memdup(const void *str, size_t size) const { return memdup_root(mem_root,str,size); } inline void *memdup_w_gap(const void *str, size_t size, size_t gap) const diff --git a/sql/sql_table.cc b/sql/sql_table.cc --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -9869,26 +9869,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];

This is generally incorrect. User specified keys are in table->s->key_info[] Keys in table->key_info[] can be modified to match what indexes are actually created, in particular, for long uniques table->key_info[] differs from table->s->key_info[]

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; } } @@ -9896,25 +9891,63 @@ 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.field && !(c.field->flags & AUTO_INCREMENT_FLAG)) + return false; + break; + } + } + return true; +} + +static +const char *online_alter_check_supported(const THD *thd, + const Alter_info *alter_info, + const TABLE *table, bool *online) { - if (c.create_if_not_exists // check IF EXISTS option - && table->find_field_by_name(&c.change) == NULL) - continue; - online= false; + DBUG_ASSERT(!table->s->tmp_table);

that's confusing. *online can be false here already, then return "DROP SYSTEM VERSIONING" will be wrong and you use some other condition to decide whether "DROP SYSTEM VERSIONING" is correct or not. it'd be much clearer not to call online_alter_check_supported in this case. and add here DBUG_ASSERT(*online);

+ + *online= *online && (alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) == 0;

and then you won't need to use && above, just as you don't use it below

+ 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 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; } - break; + return NULL; } } - return online; + + *online= online_alter_check_autoinc(thd, alter_info, table); + if (!*online) + return "CHANGE COLUMN ... AUTO_INCREMENT"; + + return NULL; }

@@ -10979,14 +10994,14 @@ do_continue:;

if (!table->s->tmp_table) { - // COPY algorithm doesn't work with concurrent writes. + auto *reason= online_alter_check_supported(thd, alter_info, table, &online); + // COPY algorithm works with concurrent writes only when online is true.

as I wrote above, better put it inside if (online) {

if (!online && alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE) { + DBUG_ASSERT(reason); 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; }

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org