List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:October 9 2007 3:03pm
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-09 12:02:59-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 (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@stripped, 2007-10-09 12:02:57-03:00, davi@stripped +11 -1
    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-09 12:02:57-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-09 12:02:57 -03:00
@@ -1745,7 +1745,13 @@ 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,
+    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->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 -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-09 12:02:57 -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;
+  /*
+    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)
Thread
bk commit into 5.0 tree (davi:1.2528) BUG#31409Davi Arnaut9 Oct
  • Re: bk commit into 5.0 tree (davi:1.2528) BUG#31409Dmitri Lenev9 Oct