From 44ca37ef17f9cf245534d30cef17b24187ac962b Mon Sep 17 00:00:00 2001 From: Nikita Malyavin Date: Thu, 27 Jul 2023 15:26:32 +0400 Subject: [PATCH] MDEV-31631 Adding auto-increment to table with history online misbehaves Adding an auto_increment column online leads to an undefined behavior. Basically any DEFAULTs that depend on a row order in the table, or on the non-deterministic (in scope of the ALTER TABLE statement) function is UB. For example, NOW() is considered generally non-deterministic (Item_func_now_utc is marked with VCOL_NON_DETERMINISTIC), but it's fixed in scope of a single statement. Same for any other function that depends only on the session/status vars apart from its arguments. Only two UB cases are known: * adding new AUTO_INCREMENT column. Modifying the existing column may be fine under certain circumstances, see MDEV-31058. * adding new column with DEFAULT(nextval(...)). Modifying the existing column is possible, since its value will be always present in the online event, except for the NULL -> NOT NULL modification --- mysql-test/main/alter_table_online.result | 14 ++++++++ mysql-test/main/alter_table_online.test | 18 +++++++++++ .../main/alter_table_online_debug.result | 17 ---------- mysql-test/main/alter_table_online_debug.test | 32 ++++++++++--------- sql/sql_table.cc | 27 ++++++++++++++++ 5 files changed, 76 insertions(+), 32 deletions(-) diff --git a/mysql-test/main/alter_table_online.result b/mysql-test/main/alter_table_online.result index 8e17ded7bba2e..7928222a428be 100644 --- a/mysql-test/main/alter_table_online.result +++ b/mysql-test/main/alter_table_online.result @@ -224,3 +224,17 @@ alter table s engine=Aria, lock=none; ERROR 0A000: LOCK=NONE is not supported. Reason: SEQUENCE. Try LOCK=SHARED alter table s engine=Aria; drop sequence s; +# MDEV-31631 Adding auto-increment column to a table with history online +# behaves differently from non-online +create sequence s; +create table t1(a int, x int NULL default(nextval(s))); +alter table t1 add b int default (nextval(s)), lock=none; +ERROR 0A000: LOCK=NONE is not supported. Reason: Function or expression 'NEXTVAL()' cannot be used in the DEFAULT clause of `b`. Try LOCK=SHARED +alter table t1 add b int primary key auto_increment, lock=none; +ERROR 0A000: LOCK=NONE is not supported. Reason: ADD COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED +create table t2(a int, b int NULL default(nextval(s))); +alter table t2 modify b int not null default (nextval(s)), lock=none; +ERROR 0A000: LOCK=NONE is not supported. Reason: Function or expression 'NEXTVAL()' cannot be used in the DEFAULT clause of `b`. Try LOCK=SHARED +drop table t2; +drop table t1; +drop sequence s; diff --git a/mysql-test/main/alter_table_online.test b/mysql-test/main/alter_table_online.test index 792ea95926fb5..35e0105489082 100644 --- a/mysql-test/main/alter_table_online.test +++ b/mysql-test/main/alter_table_online.test @@ -228,3 +228,21 @@ create sequence s engine=MyISAM; alter table s engine=Aria, lock=none; alter table s engine=Aria; drop sequence s; + + +--echo # MDEV-31631 Adding auto-increment column to a table with history online +--echo # behaves differently from non-online +create sequence s; +create table t1(a int, x int NULL default(nextval(s))); +--error ER_ALTER_OPERATION_NOT_SUPPORTED_REASON +alter table t1 add b int default (nextval(s)), lock=none; +--error ER_ALTER_OPERATION_NOT_SUPPORTED_REASON +alter table t1 add b int primary key auto_increment, lock=none; + +create table t2(a int, b int NULL default(nextval(s))); +--error ER_ALTER_OPERATION_NOT_SUPPORTED_REASON +alter table t2 modify b int not null default (nextval(s)), lock=none; + +drop table t2; +drop table t1; +drop sequence s; diff --git a/mysql-test/main/alter_table_online_debug.result b/mysql-test/main/alter_table_online_debug.result index ab9417e5fc86e..53c4ca1bc66ec 100644 --- a/mysql-test/main/alter_table_online_debug.result +++ b/mysql-test/main/alter_table_online_debug.result @@ -990,23 +990,6 @@ set debug_sync= reset; # set @old_dbug=@@debug_dbug; set debug_dbug="+d,rpl_report_chosen_key"; -create table t (a int); -insert into t values (10),(20),(30); -set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for goforit'; -alter table t add pk int auto_increment primary key, algorithm=copy, lock=none; -connection con2; -set debug_sync= 'now wait_for downgraded'; -delete from t where a = 20; -update t set a = a + 1 where a = 10; -set debug_sync= 'now signal goforit'; -connection default; -Warnings: -Note 1105 Key chosen: -1 -Note 1105 Key chosen: -1 -select * from t; -a pk -11 1 -30 3 # # Add clumsy DEFAULT # diff --git a/mysql-test/main/alter_table_online_debug.test b/mysql-test/main/alter_table_online_debug.test index b486eb9fb429a..bd97455533751 100644 --- a/mysql-test/main/alter_table_online_debug.test +++ b/mysql-test/main/alter_table_online_debug.test @@ -1172,21 +1172,23 @@ set debug_sync= reset; set @old_dbug=@@debug_dbug; set debug_dbug="+d,rpl_report_chosen_key"; -create table t (a int); -insert into t values (10),(20),(30); - -set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for goforit'; ---send -alter table t add pk int auto_increment primary key, algorithm=copy, lock=none; ---connection con2 -set debug_sync= 'now wait_for downgraded'; -delete from t where a = 20; -update t set a = a + 1 where a = 10; -set debug_sync= 'now signal goforit'; - ---connection default ---reap -select * from t; +# UB +# +# create table t (a int); +# insert into t values (10),(20),(30); +# +# set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for goforit'; +# --send +# alter table t add pk int auto_increment primary key, algorithm=copy, lock=none; +# --connection con2 +# set debug_sync= 'now wait_for downgraded'; +# delete from t where a = 20; +# update t set a = a + 1 where a = 10; +# set debug_sync= 'now signal goforit'; +# +# --connection default +# --reap +# select * from t; --echo # --echo # Add clumsy DEFAULT diff --git a/sql/sql_table.cc b/sql/sql_table.cc index e536ccdf3fb20..63eb5e4e70e58 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -9953,6 +9953,33 @@ const char *online_alter_check_supported(const THD *thd, } } + for (auto &c: alter_info->create_list) + { + *online= c.field || !(c.flags & AUTO_INCREMENT_FLAG); + if (!*online) + return "ADD COLUMN ... AUTO_INCREMENT"; + + auto *def= c.default_value; + *online= !(def && def->flags & VCOL_NEXTVAL + // either it's a new field, or a NULL -> NOT NULL change + && (!c.field || (!(c.field->flags & NOT_NULL_FLAG) + && (c.flags & NOT_NULL_FLAG)))); + if (!*online) + { + if (alter_info->requested_lock != Alter_info::ALTER_TABLE_LOCK_NONE) + return NULL; // Avoid heavy string op + const char *fmt= ER_THD(thd, ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED); + + LEX_CSTRING dflt{STRING_WITH_LEN("DEFAULT")}; + LEX_CSTRING nxvl{STRING_WITH_LEN("NEXTVAL()")}; + size_t len= strlen(fmt) + nxvl.length + c.field_name.length + dflt.length; + char *resp= (char*)thd->alloc(len); + // expression %s cannot be used in the %s clause of %`s + my_snprintf(resp, len, fmt, nxvl.str, dflt.str, c.field_name.str); + return resp; + } + } + *online= online_alter_check_autoinc(thd, alter_info, table); if (!*online) return "CHANGE COLUMN ... AUTO_INCREMENT";