Bug#31409 RENAME TABLE causes server crash or deadlock when used with HANDLER statements

This deadlock occurs when a client issues a HANDLER ... OPEN statement
that tries to open a table that has a pending name-lock on it by another
client that also needs a name-lock on some other table which is already
open and associated to a HANDLER instance owned by the first client.
The deadlock happens because the open_table() function will back-off
and wait until the name-lock goes away, causing a circular wait if some
other name-lock is also pending for one of the open HANDLER tables.

Such situation, for example, can be easily repeated by issuing a RENAME
TABLE command in such a way that the existing table is already open
as a HANDLER table by another client and this client tries to open
a HANDLER to the new table name.

The solution is to allow handler tables with older versions (marked for
flush) to be closed before waiting for the name-lock completion. This is
safe because no other name-lock can be issued between the flush and the
check for pending name-locks.

The test case for this bug is going to be committed into 5.1 because it
requires a test feature only avaiable in 5.1 (wait_condition).


sql/sql_base.cc:
  Improve comments in the open_table() function, stating the importance
  of the handler tables flushing for the back-off process.
sql/sql_handler.cc:
  Allows handler tables flushes when opening new tables in order to avoid
  potential deadlocks. Add comments explaining the importance of the flush.

2 files changed, 39 insertions, 20 deletions

diff --git a/sql/sql_base.cc b/sql/sql_base.cc
index 289924ff418..905190cb9cd 100644
--- a/sql/sql_base.cc
+++ b/sql/sql_base.cc
@@ -1745,7 +1745,13 @@ TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
     DBUG_RETURN(0);
   }
 
-  /* close handler tables which are marked for flush */
+  /*
+    In order for the back off and re-start process to work properly,
+    handler tables having old versions (due to FLUSH TABLES or pending
+    name-lock) MUST be closed. This is specially important if a name-lock
+    is pending for any table of the handler_tables list, otherwise a
+    deadlock may occur.
+  */
   if (thd->handler_tables)
     mysql_ha_flush(thd, (TABLE_LIST*) NULL, MYSQL_HA_REOPEN_ON_USAGE, TRUE);
 
@@ -1810,6 +1816,10 @@ TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
         table->db_stat == 0 signals wait_for_locked_table_names
         that the tables in question are not used any more. See
         table_is_used call for details.
+
+        Notice that HANDLER tables were already taken care of by
+        the earlier call to mysql_ha_flush() in this same critical
+        section.
       */
       close_old_data_files(thd,thd->open_tables,0,0);
       /*
diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc
index 89090c9fa63..e87381dd49c 100644
--- a/sql/sql_handler.cc
+++ b/sql/sql_handler.cc
@@ -182,7 +182,7 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen)
   char          *db, *name, *alias;
   uint          dblen, namelen, aliaslen, counter;
   int           error;
-  TABLE         *backup_open_tables, *backup_handler_tables;
+  TABLE         *backup_open_tables;
   DBUG_ENTER("mysql_ha_open");
   DBUG_PRINT("enter",("'%s'.'%s' as '%s'  reopen: %d",
                       tables->db, tables->table_name, tables->alias,
@@ -211,14 +211,20 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen)
     }
   }
 
-  /* save open_ and handler_ tables state */
-  backup_open_tables= thd->open_tables;
-  backup_handler_tables= thd->handler_tables;
+  /*
+    Save and reset the open_tables list so that open_tables() won't
+    be able to access (or know about) the previous list. And on return
+    from open_tables(), thd->open_tables will contain only the opened
+    table.
+
+    The thd->handler_tables list is kept as-is to avoid deadlocks if
+    open_table(), called by open_tables(), needs to back-off because
+    of a pending name-lock on the table being opened.
 
-  /* no pre-opened tables */
+    See open_table() back-off comments for more details.
+  */
+  backup_open_tables= thd->open_tables;
   thd->open_tables= NULL;
-  /* to avoid flushes */
-  thd->handler_tables= NULL;
 
   /*
     open_tables() will set 'tables->table' if successful.
@@ -231,9 +237,12 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen)
   error= open_tables(thd, &tables, &counter, 0);
 
   /* restore the state and merge the opened table into handler_tables list */
-  thd->handler_tables= thd->open_tables ?
-                       thd->open_tables->next= backup_handler_tables,
-                       thd->open_tables : backup_handler_tables;
+  if (thd->open_tables)
+  {
+    thd->open_tables->next= thd->handler_tables;
+    thd->handler_tables= thd->open_tables;
+  }
+
   thd->open_tables= backup_open_tables;
 
   if (error)
@@ -360,7 +369,7 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables,
                    ha_rows select_limit_cnt, ha_rows offset_limit_cnt)
 {
   TABLE_LIST    *hash_tables;
-  TABLE         *table, *backup_open_tables, *backup_handler_tables;
+  TABLE         *table, *backup_open_tables;
   MYSQL_LOCK    *lock;
   List<Item>	list;
   Protocol	*protocol= thd->protocol;
@@ -437,20 +446,20 @@ retry:
   }
   tables->table=table;
 
-  /* save open_ and handler_ tables state */
+  /* save open_tables state */
   backup_open_tables= thd->open_tables;
-  backup_handler_tables= thd->handler_tables;
-
-  /* no pre-opened tables */
-  thd->open_tables= NULL;
-  /* to avoid flushes */
-  thd->handler_tables= NULL;
+  /*
+    mysql_lock_tables() needs thd->open_tables to be set correctly to
+    be able to handle aborts properly. When the abort happens, it's
+    safe to not protect thd->handler_tables because it won't close any
+    tables.
+  */
+  thd->open_tables= thd->handler_tables;
 
   lock= mysql_lock_tables(thd, &tables->table, 1,
                           MYSQL_LOCK_NOTIFY_IF_NEED_REOPEN, &need_reopen);
 
   /* restore previous context */
-  thd->handler_tables= backup_handler_tables;
   thd->open_tables= backup_open_tables;
 
   if (need_reopen)