List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:October 6 2007 4:22am
Subject:bk commit into 5.0 tree (davi:1.2528) BUG#31409
View as plain text  
Below is the list of changes that have just been committed into a local
5.0 repository of davi. When davi does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet@stripped, 2007-10-06 01:21:55-03:00, davi@stripped +2 -0
  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 (makred 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@stripped, 2007-10-06 01:21:52-03:00, davi@stripped +8 -2
    Improve comments in the open_table() function, stating the importance
    of the handler tables flushing for the back-off process.

  sql/sql_handler.cc@stripped, 2007-10-06 01:21:53-03:00, davi@stripped +28 -19
    Allows handler tables flushes when opening new tables in order to avoid
    potential deadlocks. Add comments explaining the importance of the flush.

diff -Nrup a/sql/sql_base.cc b/sql/sql_base.cc
--- a/sql/sql_base.cc	2007-08-24 08:28:10 -03:00
+++ b/sql/sql_base.cc	2007-10-06 01:21:52 -03:00
@@ -1745,7 +1745,12 @@ TABLE *open_table(THD *thd, TABLE_LIST *
     DBUG_RETURN(0);
   }
 
-  /* close handler tables which are marked for flush */
+  /*
+    In order for the back off and re-start process to work properly, the
+    handler tables which are marked for flush (with older versions) 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);
 
@@ -1809,7 +1814,8 @@ TABLE *open_table(THD *thd, TABLE_LIST *
         table->s->version (this is an optimization (?)).
         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.
+        table_is_used call for details. Stale HANDLER tables were
+        already closed under this same critical section.
       */
       close_old_data_files(thd,thd->open_tables,0,0);
       /*
diff -Nrup a/sql/sql_handler.cc b/sql/sql_handler.cc
--- a/sql/sql_handler.cc	2007-10-04 17:34:38 -03:00
+++ b/sql/sql_handler.cc	2007-10-06 01:21:53 -03:00
@@ -182,7 +182,7 @@ bool mysql_ha_open(THD *thd, TABLE_LIST 
   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 
     }
   }
 
-  /* 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 
   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 
                    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;
+  /* not only temporary tables */
+  thd->open_tables= thd->handler_tables;
 
+  /*
+    Lock table, but if it fails don't close any tables. That's why the
+    thd->handler_table list is not protected here, mysql_lock_tables
+    won't touch it.
+  */
   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)
Thread
bk commit into 5.0 tree (davi:1.2528) BUG#31409Davi Arnaut6 Oct