diff --git a/mysql-test/r/lock.result b/mysql-test/r/lock.result index c60ec4bc1bd..46ce618b99b 100644 --- a/mysql-test/r/lock.result +++ b/mysql-test/r/lock.result @@ -282,5 +282,42 @@ insert into t1 values (1); # Ensure that metadata locks held by the transaction are released. drop table t1; # +# Bug#45035 " Altering table under LOCK TABLES results in +# "Error 1213 Deadlock found..." +# +# When reopening tables under LOCK TABLES after ALTER TABLE, +# 6.0 used to be taking thr_lock locks one by one, and +# that would lead to a lock conflict. +# Check that taking all locks at once works. +# +drop table if exists t1; +create table t1 (i int); +lock tables t1 write, t1 as a read, t1 as b read; +alter table t1 add column j int; +unlock tables; +drop table t1; +create temporary table t1 (i int); +# +# This is just for test coverage purposes, +# when this is allowed, remove the --error. +# +lock tables t1 write, t1 as a read, t1 as b read; +ERROR HY000: Can't reopen table: 't1' +alter table t1 add column j int; +unlock tables; +drop table t1; +# +# Separate case for partitioned tables is important +# because each partition has an own thr_lock object. +# +create table t1 (i int) partition by list (i) +(partition p0 values in (1), +partition p1 values in (2,3), +partition p2 values in (4,5)); +lock tables t1 write, t1 as a read, t1 as b read; +alter table t1 add column j int; +unlock tables; +drop table t1; +# # End of 6.0 tests. # diff --git a/mysql-test/t/lock.test b/mysql-test/t/lock.test index c2aeac05cd1..eaba2693904 100644 --- a/mysql-test/t/lock.test +++ b/mysql-test/t/lock.test @@ -345,6 +345,46 @@ connection default; drop table t1; +--echo # +--echo # Bug#45035 " Altering table under LOCK TABLES results in +--echo # "Error 1213 Deadlock found..." +--echo # +--echo # When reopening tables under LOCK TABLES after ALTER TABLE, +--echo # 6.0 used to be taking thr_lock locks one by one, and +--echo # that would lead to a lock conflict. +--echo # Check that taking all locks at once works. +--echo # +--disable_warnings +drop table if exists t1; +--enable_warnings +create table t1 (i int); +lock tables t1 write, t1 as a read, t1 as b read; +alter table t1 add column j int; +unlock tables; +drop table t1; +create temporary table t1 (i int); +--echo # +--echo # This is just for test coverage purposes, +--echo # when this is allowed, remove the --error. +--echo # +--error ER_CANT_REOPEN_TABLE +lock tables t1 write, t1 as a read, t1 as b read; +alter table t1 add column j int; +unlock tables; +drop table t1; +--echo # +--echo # Separate case for partitioned tables is important +--echo # because each partition has an own thr_lock object. +--echo # +create table t1 (i int) partition by list (i) + (partition p0 values in (1), + partition p1 values in (2,3), + partition p2 values in (4,5)); +lock tables t1 write, t1 as a read, t1 as b read; +alter table t1 add column j int; +unlock tables; +drop table t1; + --echo # --echo # End of 6.0 tests. --echo # diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 4f8c6be8e7b..02871c118ca 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -2980,8 +2980,11 @@ Locked_tables_list::init_locked_tables(THD *thd) { DBUG_ASSERT(thd->locked_tables_mode == LTM_NONE); DBUG_ASSERT(m_locked_tables == NULL); + DBUG_ASSERT(m_reopen_array == NULL); + DBUG_ASSERT(m_locked_tables_count == 0); - for (TABLE *table= thd->open_tables; table; table= table->next) + for (TABLE *table= thd->open_tables; table; + table= table->next, m_locked_tables_count++) { TABLE_LIST *src_table_list= table->pos_in_table_list; char *db, *table_name, *alias; @@ -3021,7 +3024,24 @@ Locked_tables_list::init_locked_tables(THD *thd) m_locked_tables_last= &dst_table_list->next_global; table->pos_in_locked_tables= dst_table_list; } + if (m_locked_tables_count) + { + /** + Allocate an auxiliary array to pass to mysql_lock_tables() + in reopen_tables(). reopen_tables() is a critical + path and we don't want to complicate it with extra allocations. + */ + m_reopen_array= (TABLE**)alloc_root(&m_locked_tables_root, + sizeof(TABLE*) * + (m_locked_tables_count+1)); + if (m_reopen_array == NULL) + { + unlock_locked_tables(0); + return TRUE; + } + } thd->locked_tables_mode= LTM_LOCK_TABLES; + return FALSE; } @@ -3070,6 +3090,8 @@ Locked_tables_list::unlock_locked_tables(THD *thd) free_root(&m_locked_tables_root, MYF(0)); m_locked_tables= NULL; m_locked_tables_last= &m_locked_tables; + m_reopen_array= NULL; + m_locked_tables_count= 0; } @@ -3141,8 +3163,39 @@ void Locked_tables_list::unlink_from_list(THD *thd, @note This function is a no-op if we're not under LOCK TABLES. */ -void Locked_tables_list::unlink_all_closed_tables() +void Locked_tables_list:: +unlink_all_closed_tables(THD *thd, MYSQL_LOCK *lock, size_t reopen_count) { + /* If we managed to take a lock, unlock tables and free the lock. */ + if (lock) + mysql_unlock_tables(thd, lock); + /* + If a failure happened in reopen_tables(), we may have succeeded + reopening some tables, but not all. + This works when the connection was killed in mysql_lock_tables(). + */ + if (reopen_count) + { + pthread_mutex_lock(&LOCK_open); + while (reopen_count--) + { + /* + When closing the table, we must remove it + from thd->open_tables list. + We rely on the fact that open_table() that was used + in reopen_tables() always links the opened table + to the beginning of the open_tables list. + */ + DBUG_ASSERT(thd->open_tables == m_reopen_array[reopen_count]); + + thd->open_tables->pos_in_locked_tables->table= NULL; + + close_thread_table(thd, &thd->open_tables); + } + broadcast_refresh(); + pthread_mutex_unlock(&LOCK_open); + } + /* Exclude all closed tables from the LOCK TABLES list. */ for (TABLE_LIST *table_list= m_locked_tables; table_list; table_list= table_list->next_global) { @@ -3176,12 +3229,13 @@ Locked_tables_list::reopen_tables(THD *thd) { enum enum_open_table_action ot_action_unused; bool lt_refresh_unused; + size_t reopen_count= 0; + MYSQL_LOCK *lock; + MYSQL_LOCK *merged_lock; for (TABLE_LIST *table_list= m_locked_tables; table_list; table_list= table_list->next_global) { - MYSQL_LOCK *lock; - if (table_list->table) /* The table was not closed */ continue; @@ -3189,33 +3243,42 @@ Locked_tables_list::reopen_tables(THD *thd) if (open_table(thd, table_list, thd->mem_root, &ot_action_unused, MYSQL_OPEN_REOPEN)) { - unlink_all_closed_tables(); + unlink_all_closed_tables(thd, 0, reopen_count); return TRUE; } table_list->table->pos_in_locked_tables= table_list; /* See also the comment on lock type in init_locked_tables(). */ table_list->table->reginfo.lock_type= table_list->lock_type; + + DBUG_ASSERT(reopen_count < m_locked_tables_count); + m_reopen_array[reopen_count++]= table_list->table; + } + if (reopen_count) + { thd->in_lock_tables= 1; - lock= mysql_lock_tables(thd, &table_list->table, 1, + /* + We re-lock all tables with mysql_lock_tables() at once rather + than locking one table at a time because of the case + reported in Bug#45035: when the same table is present + in the list many times, thr_lock.c fails to grant READ lock + on a table that is already locked by WRITE lock, even if + WRITE lock is taken by the same thread. If READ and WRITE + lock are passed to thr_lock.c in the same list, everything + works fine. Patching legacy code of thr_lock.c is risking to + break something else. + */ + lock= mysql_lock_tables(thd, m_reopen_array, reopen_count, MYSQL_OPEN_REOPEN, <_refresh_unused); thd->in_lock_tables= 0; - if (lock) - lock= mysql_lock_merge(thd->lock, lock); - if (lock == NULL) + if (lock == NULL || (merged_lock= + mysql_lock_merge(thd->lock, lock)) == NULL) { - /* - No one's seen this branch work. Recover and report an - error just in case. - */ - pthread_mutex_lock(&LOCK_open); - close_thread_table(thd, &thd->open_tables); - pthread_mutex_unlock(&LOCK_open); - table_list->table= 0; - unlink_all_closed_tables(); - my_error(ER_LOCK_DEADLOCK, MYF(0)); + unlink_all_closed_tables(thd, lock, reopen_count); + if (! thd->killed) + my_error(ER_LOCK_DEADLOCK, MYF(0)); return TRUE; } - thd->lock= lock; + thd->lock= merged_lock; } return FALSE; } diff --git a/sql/sql_class.h b/sql/sql_class.h index 92b9f9f4611..4b6564fb9da 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1192,7 +1192,7 @@ private: Therefore, we can't allocate metadata locks on execution memory root -- as well as tables, the locks need to stay around till UNLOCK TABLES is called. - The locks are allocated in the memory root encapsulate in this + The locks are allocated in the memory root encapsulated in this class. Some SQL commands, like FLUSH TABLE or ALTER TABLE, demand that @@ -1211,10 +1211,21 @@ private: MEM_ROOT m_locked_tables_root; TABLE_LIST *m_locked_tables; TABLE_LIST **m_locked_tables_last; + /** An auxiliary array used only in reopen_tables(). */ + TABLE **m_reopen_array; + /** + Count the number of tables in m_locked_tables list. We can't + rely on thd->lock->table_count because it excludes + non-transactional temporary tables. We need to know + an exact number of TABLE objects. + */ + size_t m_locked_tables_count; public: Locked_tables_list() :m_locked_tables(NULL), - m_locked_tables_last(&m_locked_tables) + m_locked_tables_last(&m_locked_tables), + m_reopen_array(NULL), + m_locked_tables_count(0) { init_sql_alloc(&m_locked_tables_root, MEM_ROOT_BLOCK_SIZE, 0); } @@ -1228,7 +1239,9 @@ public: MEM_ROOT *locked_tables_root() { return &m_locked_tables_root; } void unlink_from_list(THD *thd, TABLE_LIST *table_list, bool remove_from_locked_tables); - void unlink_all_closed_tables(); + void unlink_all_closed_tables(THD *thd, + MYSQL_LOCK *lock, + size_t reopen_count); bool reopen_tables(THD *thd); }; diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 8fdaa2cd93a..527095f2c88 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4515,7 +4515,7 @@ static int prepare_for_repair(THD *thd, TABLE_LIST *table_list, } end: - thd->locked_tables_list.unlink_all_closed_tables(); + thd->locked_tables_list.unlink_all_closed_tables(thd, NULL, 0); if (table == &tmp_table) { pthread_mutex_lock(&LOCK_open); @@ -7597,7 +7597,7 @@ err_with_mdl: remove all references to the altered table from the list of locked tables and release the exclusive metadata lock. */ - thd->locked_tables_list.unlink_all_closed_tables(); + thd->locked_tables_list.unlink_all_closed_tables(thd, NULL, 0); if (target_mdl_request) { thd->mdl_context.release_lock(target_mdl_request->ticket);