From a228ec80e30cae59c2057b786fd3559171ce794f Mon Sep 17 00:00:00 2001 From: Aleksey Midenkov Date: Wed, 31 Aug 2022 11:55:05 +0300 Subject: [PATCH] MDEV-28956 Locking is broken if CREATE OR REPLACE fails under LOCK TABLES add_back_last_deleted_lock() was called when the lock was never removed. Lock is removed in finalize_atomic_replace() in close_all_tables_for_name(). finalize_atomic_replace() is done only for successful operation. In non-atomic codepath it drops the table first, if anything fails later we don't need to return back the lock since there is no table now. So the fix is required as well. --- mysql-test/main/create_or_replace.result | 16 ++++++++ mysql-test/main/create_or_replace.test | 17 ++++++++ mysql-test/main/create_or_replace2.result | 19 +++++++++ mysql-test/main/create_or_replace2.test | 21 ++++++++++ sql/handler.h | 2 +- sql/sql_insert.cc | 6 ++- sql/sql_table.cc | 48 ++++++++++++++++------- 7 files changed, 112 insertions(+), 17 deletions(-) diff --git a/mysql-test/main/create_or_replace.result b/mysql-test/main/create_or_replace.result index 142856d603f..d9ca4102b06 100644 --- a/mysql-test/main/create_or_replace.result +++ b/mysql-test/main/create_or_replace.result @@ -847,3 +847,19 @@ t2 CREATE TABLE `t2` ( `a` varchar(2300) DEFAULT NULL ) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 drop tables t2, t1; +# +# MDEV-28956 Locking is broken if CREATE OR REPLACE fails under LOCK TABLES +# +# Atomic CREATE OR REPLACE part: +# +create table t1 (pk int primary key) engine=innodb; +create or replace table t2 (a int primary key references t1 (pk)) engine=innodb; +lock tables t1 write, t2 write; +create or replace table t2 (c1 int not null, c1 varchar(255) ) engine=innodb; +ERROR 42S21: Duplicate column name 'c1' +select * from t1; +pk +select * from t2; +a +unlock tables; +drop tables t2, t1; diff --git a/mysql-test/main/create_or_replace.test b/mysql-test/main/create_or_replace.test index c18c8b9eb7c..316d904e591 100644 --- a/mysql-test/main/create_or_replace.test +++ b/mysql-test/main/create_or_replace.test @@ -638,3 +638,20 @@ create or replace table t2 engine aria select * from t1; select * from t2; show create table t2; drop tables t2, t1; + +--echo # +--echo # MDEV-28956 Locking is broken if CREATE OR REPLACE fails under LOCK TABLES +--echo # +--echo # Atomic CREATE OR REPLACE part: +--echo # +create table t1 (pk int primary key) engine=innodb; +create or replace table t2 (a int primary key references t1 (pk)) engine=innodb; + +lock tables t1 write, t2 write; +--error ER_DUP_FIELDNAME +create or replace table t2 (c1 int not null, c1 varchar(255) ) engine=innodb; +select * from t1; +select * from t2; +unlock tables; + +drop tables t2, t1; diff --git a/mysql-test/main/create_or_replace2.result b/mysql-test/main/create_or_replace2.result index f7bbb7417e6..dd183441e42 100644 --- a/mysql-test/main/create_or_replace2.result +++ b/mysql-test/main/create_or_replace2.result @@ -72,3 +72,22 @@ ERROR 42000: Duplicate key name 'k' create or replace table t1 (a int, b int, key k (a), key k (b)); ERROR 42000: Duplicate key name 'k' drop table t1; +# +# MDEV-28956 Locking is broken if CREATE OR REPLACE fails under LOCK TABLES +# +# Non-atomic CREATE OR REPLACE part: +# +set @saved_debug_dbug= @@session.debug_dbug; +set @@debug_dbug="+d,ddl_log_expensive_rename"; +create table t1 (pk int primary key) engine=innodb; +create or replace table t2 (a int primary key references t1 (pk)) engine=innodb; +lock tables t1 write, t2 write; +create or replace table t2 (c1 int not null, c1 varchar(255) ) engine=innodb; +ERROR 42S21: Duplicate column name 'c1' +select * from t1; +pk +select * from t2; +ERROR HY000: Table 't2' was not locked with LOCK TABLES +unlock tables; +drop tables t1; +set @@debug_dbug= @saved_debug_dbug; diff --git a/mysql-test/main/create_or_replace2.test b/mysql-test/main/create_or_replace2.test index 227aff226d5..bd82d13f1d1 100644 --- a/mysql-test/main/create_or_replace2.test +++ b/mysql-test/main/create_or_replace2.test @@ -65,3 +65,24 @@ create or replace table t1 (a int, b int, key k (a), key k (b)); --error ER_DUP_KEYNAME create or replace table t1 (a int, b int, key k (a), key k (b)); drop table t1; + +--echo # +--echo # MDEV-28956 Locking is broken if CREATE OR REPLACE fails under LOCK TABLES +--echo # +--echo # Non-atomic CREATE OR REPLACE part: +--echo # +set @saved_debug_dbug= @@session.debug_dbug; +set @@debug_dbug="+d,ddl_log_expensive_rename"; +create table t1 (pk int primary key) engine=innodb; +create or replace table t2 (a int primary key references t1 (pk)) engine=innodb; + +lock tables t1 write, t2 write; +--error ER_DUP_FIELDNAME +create or replace table t2 (c1 int not null, c1 varchar(255) ) engine=innodb; +select * from t1; +--error ER_TABLE_NOT_LOCKED +select * from t2; +unlock tables; + +drop tables t1; +set @@debug_dbug= @saved_debug_dbug; diff --git a/sql/handler.h b/sql/handler.h index 1abe66448aa..d4ea7687f15 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -2382,7 +2382,7 @@ struct HA_CREATE_INFO: public Table_scope_and_contents_source_st, } bool finalize_atomic_replace(THD *thd, TABLE_LIST *orig_table); void finalize_ddl(THD *thd, bool roll_back); - bool finalize_locked_tables(THD *thd); + bool finalize_locked_tables(THD *thd, bool operation_failed); bool make_tmp_table_list(THD *thd, TABLE_LIST **create_table, int *create_table_mode); }; diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 6fc46e5818c..163dd45f50c 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -5414,7 +5414,8 @@ bool select_create::send_eof() mysql_unlock_tables(thd, lock); } else if (atomic_replace && create_info->pos_in_locked_tables && - create_info->finalize_locked_tables(thd)) + /* send_eof() is done on success, passing false here */ + create_info->finalize_locked_tables(thd, false)) DBUG_RETURN(true); send_ok_packet(); @@ -5526,6 +5527,7 @@ void select_create::abort_result_set() thd->locked_tables_list.unlock_locked_table(thd, create_info->mdl_ticket); } else if (atomic_replace && create_info->pos_in_locked_tables) - (void) create_info->finalize_locked_tables(thd); + /* abort_result_set() is done on error, passing true here */ + (void) create_info->finalize_locked_tables(thd, true); DBUG_VOID_RETURN; } diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 0d76a50f4ce..5855c21bbb3 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4509,27 +4509,47 @@ void HA_CREATE_INFO::finalize_ddl(THD *thd, bool roll_back) } -bool HA_CREATE_INFO::finalize_locked_tables(THD *thd) +/** + Finalize operation of LOCK TABLES mode for CREATE TABLE family of commands. + + @param operation_failed Notify if the callee CREATE fails the operation + @return true on error, false on success +*/ +bool HA_CREATE_INFO::finalize_locked_tables(THD *thd, bool operation_failed) { DBUG_ASSERT(pos_in_locked_tables); DBUG_ASSERT(thd->locked_tables_mode); DBUG_ASSERT(thd->variables.option_bits & OPTION_TABLE_LOCK); - /* - Add back the deleted table and re-created table as a locked table - This should always work as we have a meta lock on the table. - */ - thd->locked_tables_list.add_back_last_deleted_lock(pos_in_locked_tables); + if (!operation_failed) + { + /* + Add back the deleted table and re-created table as a locked table + This should always work as we have a meta lock on the table. + */ + thd->locked_tables_list.add_back_last_deleted_lock(pos_in_locked_tables); + } if (thd->locked_tables_list.reopen_tables(thd, false)) { thd->locked_tables_list.unlink_all_closed_tables(thd, NULL, 0); return true; } - /* - The lock was made exclusive in create_table_impl(). We have now - to bring it back to it's orginal state - */ - TABLE *table= pos_in_locked_tables->table; - table->mdl_ticket->downgrade_lock(MDL_SHARED_NO_READ_WRITE); + if (is_atomic_replace() || !operation_failed) + { + /* + The lock was made exclusive in create_table_impl(). We have now + to bring it back to it's orginal state. + */ + TABLE *table= pos_in_locked_tables->table; + table->mdl_ticket->downgrade_lock(MDL_SHARED_NO_READ_WRITE); + } + else + { + /* + In failed non-atomic we have nothing to downgrade: + original table was deleted and the lock was already removed. + */ + DBUG_ASSERT(!pos_in_locked_tables->table); + } return false; } @@ -5278,7 +5298,7 @@ err: */ if (thd->locked_tables_mode && pos_in_locked_tables && create_info->or_replace()) - result|= (int) create_info->finalize_locked_tables(thd); + result|= (int) create_info->finalize_locked_tables(thd, result); DBUG_RETURN(result); } @@ -6023,7 +6043,7 @@ err: */ if (thd->locked_tables_mode && pos_in_locked_tables && create_info->or_replace()) - res|= (int) local_create_info.finalize_locked_tables(thd); + res|= (int) local_create_info.finalize_locked_tables(thd, res); DBUG_RETURN(res != 0); }