Hi, Nikita, comments below refer to a combined diff of both commits On May 23, Nikita Malyavin wrote:

revision-id: c457f237511 (mariadb-11.0.1-124-gc457f237511) parent(s): f4b04ec534d author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2023-05-15 21:07:50 +0300 message:

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

Two main thoughts. First, users will generally don't do show warnings after an error - mariadb already puts some useful info into a warning after an error in some (few) cases, and it's always a surprise when I write in a bug report "use SHOW WARNINGS". It's not very intuitive. And in your case it's even unnecessary, because your error message is LOCK=NONE is not supported. Reason: %s. Try LOCK=SHARED and "Reason" part is perfectly suited for an explanation that you generate, instead of a misleading "COPY algorithm requires a lock" (because it doesn't). Second thought, your messages are hard-coded and won't be translated even if the user will configure the server to use, say, spanish or Japanese error messages. See how it was solved for "COPY algorithm requires a lock", ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY. But I think the message will still be comprehensible if you'll simply print the offending sql clause as the reason, this won't need any localization. See below where I've shown what your error messages would look like if you do that. If you think that's too cryptic, then, please, use the ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY approach. Two more comments at the very end but I'll repeat them here, because they're easy to overlook: AUTO_INCREMENT instead of AUTOINC, and a test for DROP SYSTEM VERSIONING.

diff --git a/mysql-test/main/alter_table_online.result b/mysql-test/main/alter_table_online.result index 8b726ccb591..bcf7afea9df 100644 --- a/mysql-test/main/alter_table_online.result +++ b/mysql-test/main/alter_table_online.result @@ -4,6 +4,10 @@ create table t (a int); alter ignore table t add primary key (a), 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: ALTER IGNORE. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 ALTER IGNORE TABLE is incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED drop table t; # # MDEV-28771 Assertion `table->in_use&&tdc->flushed' failed after ALTER @@ -92,12 +96,20 @@ on update cascade) engine=InnoDB; insert into t2 values (1),(2),(3); alter table t2 add c int, 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: FOREIGN KEY ... ON UPDATE CASCADE. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED alter table t2 add c int, algorithm=inplace, lock=none; create or replace table t2 (b int, foreign key (b) references t1 (a) on delete set null) engine=InnoDB; alter table t2 add c int, 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: FOREIGN KEY ... ON DELETE SET NULL. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED alter table t2 add c int, algorithm=inplace, lock=none; create or replace table t2 (b int, foreign key (b) references t1 (a) @@ -117,6 +129,10 @@ b int references t1 (b) on update cascade) engine=InnoDB; insert into t2 values (1, 1),(2, 2); alter table t2 add c int, 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: FOREIGN KEY ... ON DELETE SET NULL. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED alter table t2 add c int, algorithm=copy; alter table t2 add d int, algorithm=inplace; drop table t2, t1; @@ -132,6 +148,10 @@ period for system_time (row_start, row_end)) engine=innodb with system versioning; alter table t1 add c int, 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: BIGINT GENERATED ALWAYS AS ROW_START. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Transaction-versioned tables are incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED alter table t1 add c int, algorithm=inplace; alter table t1 add d int, lock=none; set system_versioning_alter_history= default; @@ -143,6 +163,10 @@ drop table t1; create table t (a serial, b int) engine=innodb; alter table t drop a, modify b 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: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED set statement sql_mode= NO_AUTO_VALUE_ON_ZERO for alter table t drop a, modify b serial, algorithm=copy, lock=none; create or replace table t (a serial, b int) engine=innodb; @@ -157,6 +181,10 @@ t CREATE TABLE `t` ( # Only unsafe approach is fine because of possible collisions. alter table t modify a int, modify b 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: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED # # Check that we treat autoinc columns correctly modify old autoinc is # fine, adding new autoinc for existed column is unsafe. @@ -180,22 +208,38 @@ t CREATE TABLE `t` ( ) 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: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. 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: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. 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: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED alter table t add unique(b, c); # Change unique'scolumn; alter table t change b x int, 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: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED

+show warnings; +Level Code Message +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY +Error 1846 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), algorithm=copy, lock=none; diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 99570008ddb..22447db42af 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -9916,6 +9916,67 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info, return online; }

+bool online_alter_is_supported(THD *thd, const Alter_info *alter_info, + const TABLE *table) +{ + const char *reason= NULL; + List<FOREIGN_KEY_INFO> fk_list; + + if (alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) + { + reason= "DROP SYSTEM VERSIONING is";

no test for that

+ goto unsupported; + } + + if (thd->lex->ignore) + { + reason= "ALTER IGNORE TABLE is"; + goto unsupported; + } + + if (table->s->tmp_table) + { + reason= "Temporary tables are"; + goto unsupported; + } + + if (table->versioned(VERS_TRX_ID)) + { + reason= "Transaction-versioned tables are"; + goto unsupported; + } + + 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)) + { + reason= "Tables with CASCADE/SET NULL foreign keys are"; + goto unsupported; + } + } + + if (!online_alter_check_autoinc(thd, alter_info, table)) + { + reason= "Adding AUTOINC to an existing column for a table without a "

AUTO_INCREMENT, please, don't abbrev user visible messages

+ "primary key is"; + goto unsupported; + } + + return true; + +unsupported: + if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE + && alter_info->algorithm(thd) == Alter_info::ALTER_TABLE_ALGORITHM_COPY) + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, + ER_ALTER_OPERATION_NOT_SUPPORTED_REASON, + "%s incompatible with " + "LOCK=NONE, ALGORITHM=COPY", + reason); + return false; +} +

/** Alter table

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