Backport of:

----------------------------------------------------------
revno: 2617.69.2
committer: Konstantin Osipov <kostja@sun.com>
branch nick: 5.4-azalea-bugfixing
timestamp: Mon 2009-08-03 19:26:04 +0400
message:
  A fix and a test case for Bug#45035 "Altering table under LOCK TABLES
  results in "Error 1213 Deadlock found...".

  If a user had a table locked with LOCK TABLES
  for READ and for WRITE in the same connection, ALTER TABLE
  could fail.

  Root cause analysis:

  If a connection issues

  LOCK TABLE t1 write, t1 a read, t1 b read;

  the new LOCK TABLES code in 6.0 (part of WL 3726) will create
  the following list of TABLE_LIST objects
  (thd->locked_tables_list->m_locked_tables):

  {"t1" "b" tl_read_no_insert}, {"t1" "a" tl_read_no_insert},
  {"t1" "t1" tl_write }

  Later on, when we try to ALTER table t1, mysql_alter_table()
  closes all TABLE instances and releases its thr_lock locks,
  keeping only an exclusive metadata lock on t1.

  But when ALTER is finished, Locked_table_list::reopen_tables()
  tries to restore the original list of open and locked tables.

  Before this patch, it used to do so one by one:
  Open t1 b, get TL_READ_NO_INSERT lock,
  Open t1 a, get TL_READ_NO_INSERT lock
  Open t1, try to get TL_WRITE lock, deadlock.

  The cause of the deadlock is that thr_lock.c doesn't
  resolve the situation when the read list only consists
  of locks taken by the same thread, followed by this very
  thread trying to take a WRITE lock. Indeed, since
  thr_lock_multi always gets a sorted list of locks,
  WRITE locks always precede READ locks in the list
  to lock.

  Don't try to fix thr_lock.c deficiency, keep this
  code simple.
  Instead, try to take all thr_lock locks at once
  in ::reopen_tables().

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.
 #

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 #

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, &lt_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;
 }

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);
 };
 

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);