bk commit into 5.0 tree (ingo:1.2127) BUG#16986

[ingo]

Below is the list of changes that have just been committed into a local
5.0 repository of mydev. When mydev 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
  1.2127 06/05/17 22:24:37 ingo@stripped +6 -0
  Bug#16986 - Deadlock condition with MyISAM tables
  
  A deadlock was possible with explicit write locks
  in conjunction with table administrations like OPTIMIZE.
  
  A second thread had to access two of the locked tables
  between the LOCK TABLES and the OPTIMIZE of the first
  thread. It did not release both of the tables when it 
  detected the request for refresh. So the first thread 
  could block and wait for the close of the other table.
  
  I do now care for closing all tables if a thread needs 
  to wait for one table.
  
  Using the stress test suite I found two other deadlock
  paths. One is handled in Bug 19815 (CREATE/RENAME/DROP 
  DATABASE can deadlock on a global read lock). The other
  one is included in the patch. It was also related to the
  global read lock.

  sql/sql_table.cc
    1.309 06/05/17 22:24:32 ingo@stripped +2 -2
    Bug#16986 - Deadlock condition with MyISAM tables
    Changed the call of open_table() for the new declaration.

  sql/sql_base.cc
    1.335 06/05/17 22:24:32 ingo@stripped +90 -19
    Bug#16986 - Deadlock condition with MyISAM tables
    Changed open_table() to use the new flag value 
    MYSQL_LOCK_NO_WAIT_FOR_FLUSH to suppress the
    wait_for_refresh() and return *refresh= 2 instead.
    Changed open_tables() to supply the new flag value
    MYSQL_LOCK_NO_WAIT_FOR_FLUSH to open_table() and
    use the condition refresh > 1 to call the suppressed
    wait_for_refresh() after closing all tables.
    Unrelated changes:
    Extended comments, minor style improvements, 
    additional trace prints, and fixed a possible 
    hang with FLUSH TABLES WITH READ LOCK.

  sql/sp.cc
    1.113 06/05/17 22:24:32 ingo@stripped +2 -1
    Bug#16986 - Deadlock condition with MyISAM tables
    Changed the call of open_table() for the new declaration.

  sql/mysql_priv.h
    1.386 06/05/17 22:24:31 ingo@stripped +5 -2
    Bug#16986 - Deadlock condition with MyISAM tables
    Changed the declaration of open_table() to allow
    the return of multiple values for 'refresh'.
    Changed the declaration of close_cached_tables()
    to reflect the original name of the if_wait_for_refresh
    parameter.
    Added the flag value MYSQL_LOCK_NO_WAIT_FOR_FLUSH for use 
    in open_tables() and open_table().

  mysql-test/t/lock_multi.test
    1.14 06/05/17 22:24:31 ingo@stripped +31 -0
    Bug#16986 - Deadlock condition with MyISAM tables
    The test case

  mysql-test/r/lock_multi.result
    1.16 06/05/17 22:24:31 ingo@stripped +16 -0
    Bug#16986 - Deadlock condition with MyISAM tables
    The test result

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	ingo
# Host:	chilla.local
# Root:	/home/mydev/mysql-5.0-bug16986

--- 1.385/sql/mysql_priv.h	2006-05-09 08:26:19 +02:00
+++ 1.386/sql/mysql_priv.h	2006-05-17 22:24:31 +02:00
@@ -775,7 +775,7 @@
 bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create);
 TABLE *open_ltable(THD *thd, TABLE_LIST *table_list, thr_lock_type update);
 TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT* mem,
-		  bool *refresh, uint flags);
+		  int *refresh, uint flags);
 bool reopen_name_locked_table(THD* thd, TABLE_LIST* table);
 TABLE *find_locked_table(THD *thd, const char *db,const char *table_name);
 bool reopen_table(TABLE *table,bool locked);
@@ -1009,7 +1009,8 @@
 bool remove_table_from_cache(THD *thd, const char *db, const char *table,
                              uint flags);
 
-bool close_cached_tables(THD *thd, bool wait_for_refresh, TABLE_LIST *tables);
+bool close_cached_tables(THD *thd, bool if_wait_for_refresh,
+                         TABLE_LIST *tables);
 void copy_field_from_tmp_record(Field *field,int offset);
 bool fill_record(THD *thd, Field **field, List<Item> &values,
                  bool ignore_errors);
@@ -1319,6 +1320,8 @@
 #define MYSQL_LOCK_IGNORE_GLOBAL_READ_LOCK      0x0001
 #define MYSQL_LOCK_IGNORE_FLUSH                 0x0002
 #define MYSQL_LOCK_NOTIFY_IF_NEED_REOPEN        0x0004
+/* Used with open_tables() */
+#define MYSQL_LOCK_NO_WAIT_FOR_FLUSH            0x0008
 
 void mysql_unlock_tables(THD *thd, MYSQL_LOCK *sql_lock);
 void mysql_unlock_read_tables(THD *thd, MYSQL_LOCK *sql_lock);

--- 1.334/sql/sql_base.cc	2006-04-27 17:20:41 +02:00
+++ 1.335/sql/sql_base.cc	2006-05-17 22:24:32 +02:00
@@ -1046,8 +1046,18 @@
 
 
 /*
-   When we call the following function we must have a lock on
-   LOCK_open ; This lock will be unlocked on return.
+  Wait for a refresh signal.
+
+  SYNOPSIS
+    wait_for_refresh()
+      thd                       Thread handle.
+
+  NOTE
+    When we call the following function we must have a lock on
+    LOCK_open ; This lock will be unlocked on return.
+
+  RETURN
+    void
 */
 
 void wait_for_refresh(THD *thd)
@@ -1160,6 +1170,9 @@
                           MYSQL_LOCK_IGNORE_FLUSH - Open table even if
                           someone has done a flush or namelock on it.
                           No version number checking is done.
+                          MYSQL_LOCK_NO_WAIT_FOR_FLUSH - Do not wait for
+                          a flush signal. Caller will do it if *refresh
+                          returns greater than 1.
 
   IMPLEMENTATION
     Uses a cache of open tables to find a table not in use.
@@ -1172,7 +1185,7 @@
 
 
 TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
-		  bool *refresh, uint flags)
+		  int *refresh, uint flags)
 {
   reg1	TABLE *table;
   char	key[MAX_DBKEY_LENGTH];
@@ -1180,10 +1193,12 @@
   char	*alias= table_list->alias;
   HASH_SEARCH_STATE state;
   DBUG_ENTER("open_table");
+  DBUG_PRINT("info",("Opening '%s.%s'", table_list->db,
+                     table_list->table_name));
 
   /* find a unused table in the open table cache */
   if (refresh)
-    *refresh=0;
+    *refresh= 0;
 
   /* an open table operation needs a lot of the stack space */
   if (check_stack_overrun(thd, STACK_MIN_SIZE_FOR_OPEN, (char *)&alias))
@@ -1325,14 +1340,25 @@
 
   VOID(pthread_mutex_lock(&LOCK_open));
 
-  if (!thd->open_tables)
+  /*
+    If a FLUSH TABLES WITH READ LOCK is in its flush phase, it has already
+    acquired the global read lock, incremented refresh_version, and may
+    be waiting for a flush signal. In this situation it releases LOCK_open.
+    So if we got here with trying a write operation and a global read lock
+    set, do not copy the refresh_version, as we could then block in
+    wait_if_global_read_lock() later. If, however, we have the read lock
+    ourselves, we ought to detect the lock conflict there.
+  */
+  if (! thd->open_tables &&
+      (! global_read_lock || thd->global_read_lock ||
+       (table_list->lock_type < TL_WRITE_ALLOW_WRITE)))
     thd->version=refresh_version;
   else if ((thd->version != refresh_version) &&
            ! (flags & MYSQL_LOCK_IGNORE_FLUSH))
   {
     /* Someone did a refresh while thread was opening tables */
     if (refresh)
-      *refresh=1;
+      *refresh= 1;
     VOID(pthread_mutex_unlock(&LOCK_open));
     DBUG_RETURN(0);
   }
@@ -1341,12 +1367,18 @@
   if (thd->handler_tables)
     mysql_ha_flush(thd, (TABLE_LIST*) NULL, MYSQL_HA_REOPEN_ON_USAGE, TRUE);
 
+  /* Search for an unused table in the open_cache. */
   for (table= (TABLE*) hash_first(&open_cache, (byte*) key, key_length,
                                   &state);
        table && table->in_use ;
        table= (TABLE*) hash_next(&open_cache, (byte*) key, key_length,
                                  &state))
   {
+    /*
+      Found a table in use. Check for refresh. Even if the table does
+      belong to a different thread, the table must not be reopened until
+      the using thread is done with it.
+    */
     if (table->s->version != refresh_version)
     {
       if (flags & MYSQL_LOCK_IGNORE_FLUSH)
@@ -1359,21 +1391,39 @@
       /*
         There is a refresh in progress for this table
         Wait until the table is freed or the thread is killed.
+        But do not wait here, when called from open_tables() as we can
+        have more tables open, which block others from releasing this
+        table. open_tables() will close all our tables, wait for
+        refresh and retry. (Bug #16986)
       */
       close_old_data_files(thd,thd->open_tables,0,0);
+      if (refresh)
+        *refresh= 1;
       if (table->in_use != thd)
-	wait_for_refresh(thd);
-      else
       {
-	VOID(pthread_mutex_unlock(&LOCK_open));
+        if (flags & MYSQL_LOCK_NO_WAIT_FOR_FLUSH)
+        {
+          /* Tell the caller if we need to wait for flush. */
+          if (refresh)
+            *refresh= 2;
+          VOID(pthread_mutex_unlock(&LOCK_open));
+        }
+        else
+        {
+          /* This unlocks LOCK_open. */
+          wait_for_refresh(thd);
+        }
       }
-      if (refresh)
-	*refresh=1;
+      else
+        VOID(pthread_mutex_unlock(&LOCK_open));
+      DBUG_PRINT("info", ("Failing open on refresh"));
       DBUG_RETURN(0);
     }
   }
   if (table)
   {
+    /* Here table->in_use == NULL. Hence it must be in the unused chain. */
+    DBUG_PRINT("info", ("Using unused table"));
     if (table == unused_tables)
     {						// First unused
       unused_tables=unused_tables->next;	// Remove from link
@@ -1392,6 +1442,7 @@
       VOID(hash_delete(&open_cache,(byte*) unused_tables)); /* purecov: tested */
 
     /* make a new table */
+    DBUG_PRINT("info", ("Making new table"));
     if (!(table=(TABLE*) my_malloc(sizeof(*table),MYF(MY_WME))))
     {
       VOID(pthread_mutex_unlock(&LOCK_open));
@@ -2052,7 +2103,7 @@
 int open_tables(THD *thd, TABLE_LIST **start, uint *counter, uint flags)
 {
   TABLE_LIST *tables;
-  bool refresh;
+  int refresh;
   int result=0;
   MEM_ROOT new_frm_mem;
   /* Also used for indicating that prelocking is need */
@@ -2131,9 +2182,15 @@
       DBUG_RETURN(-1);
     }
     (*counter)++;
-    
-    if (!tables->table &&
-	!(tables->table= open_table(thd, tables, &new_frm_mem, &refresh, flags)))
+
+    /*
+      Tell open_table() not to wait if a refresh is required.
+      All tables will be closed and reopend in this case.
+      Otherwise a deadlock might happen. (Bug #16986)
+    */
+    if (! tables->table &&
+        ! (tables->table= open_table(thd, tables, &new_frm_mem, &refresh,
+                                     flags | MYSQL_LOCK_NO_WAIT_FOR_FLUSH)))
     {
       free_root(&new_frm_mem, MYF(MY_KEEP_PREALLOC));
 
@@ -2182,6 +2239,20 @@
         if (query_tables_last_own)
           thd->lex->mark_as_requiring_prelocking(query_tables_last_own);
         close_tables_for_reopen(thd, start);
+
+        /*
+          If open_table() detected an ongoing refresh for this table,
+          we need to wait for the refresh signal after we closed all
+          tables that we opened. open_table() omits this wait if the
+          MYSQL_LOCK_NO_WAIT_FOR_FLUSH flag is set.
+        */
+        if (refresh > 1)
+        {
+          /* Don't run in a spin loop. */
+          DBUG_PRINT("info", ("Waiting for refresh"));
+          VOID(pthread_mutex_lock(&LOCK_open));
+          wait_for_refresh(thd);
+        }
 	goto restart;
       }
       result= -1;				// Fatal error
@@ -2319,7 +2390,8 @@
 TABLE *open_ltable(THD *thd, TABLE_LIST *table_list, thr_lock_type lock_type)
 {
   TABLE *table;
-  bool refresh;
+  bool refresh_dummy;
+  int refresh;
   DBUG_ENTER("open_ltable");
 
   thd->proc_info="Opening table";
@@ -2352,7 +2424,7 @@
       DBUG_ASSERT(thd->lock == 0);	// You must lock everything at once
       if ((table->reginfo.lock_type= lock_type) != TL_UNLOCK)
 	if (! (thd->lock= mysql_lock_tables(thd, &table_list->table, 1, 0,
-                                            &refresh)))
+                                            &refresh_dummy)))
 	  table= 0;
     }
   }
@@ -4395,7 +4467,6 @@
     tables	  Table list (select_lex->table_list)
     conds	  Condition of current SELECT (can be changed by VIEW)
     leaves        List of join table leaves list (select_lex->leaf_tables)
-    refresh       It is onle refresh for subquery
     select_insert It is SELECT ... INSERT command
 
   NOTE
@@ -5124,7 +5195,7 @@
   char key[MAX_DBKEY_LENGTH];
   uint key_length;
   TABLE *table;
-  bool result=0, signalled= 0;
+  bool result, signalled;
   DBUG_ENTER("remove_table_from_cache");
 
   key_length=(uint) (strmov(strmov(key,db)+1,table_name)-key)+1;

--- 1.308/sql/sql_table.cc	2006-05-04 06:59:24 +02:00
+++ 1.309/sql/sql_table.cc	2006-05-17 22:24:32 +02:00
@@ -1862,7 +1862,7 @@
                             create_info, *extra_fields, *keys, 0,
                             select_field_count))
     {
-      if (! (table= open_table(thd, create_table, thd->mem_root, (bool*) 0,
+      if (! (table= open_table(thd, create_table, thd->mem_root, (int*) 0,
                                MYSQL_LOCK_IGNORE_FLUSH)))
         quick_rm_table(create_info->db_type, create_table->db,
                        table_case_name(create_info, create_table->table_name));
@@ -3774,7 +3774,7 @@
       bzero((void*) &tbl, sizeof(tbl));
       tbl.db= new_db;
       tbl.table_name= tbl.alias= tmp_name;
-      new_table= open_table(thd, &tbl, thd->mem_root, (bool*) 0,
+      new_table= open_table(thd, &tbl, thd->mem_root, (int*) 0,
                             MYSQL_LOCK_IGNORE_FLUSH);
     }
     else

--- 1.15/mysql-test/r/lock_multi.result	2005-09-15 21:17:37 +02:00
+++ 1.16/mysql-test/r/lock_multi.result	2006-05-17 22:24:31 +02:00
@@ -43,3 +43,19 @@
 a	int(11)	YES		NULL	
 unlock tables;
 drop table t1;
+use mysql;
+LOCK TABLES columns_priv WRITE, db WRITE, host WRITE, user WRITE;
+FLUSH TABLES;
+use mysql;
+ SELECT user.Select_priv FROM user, db WHERE user.user = db.user LIMIT 1;
+OPTIMIZE TABLES columns_priv, db, host, user;
+Table	Op	Msg_type	Msg_text
+mysql.columns_priv	optimize	status	OK
+mysql.db	optimize	status	OK
+mysql.host	optimize	status	OK
+mysql.user	optimize	status	OK
+UNLOCK TABLES;
+Select_priv
+N
+use test;
+use test;

--- 1.13/mysql-test/t/lock_multi.test	2005-09-15 21:17:37 +02:00
+++ 1.14/mysql-test/t/lock_multi.test	2006-05-17 22:24:31 +02:00
@@ -107,3 +107,34 @@
 connection locker;
 unlock tables;
 drop table t1;
+
+#
+# Bug#16986 - Deadlock condition with MyISAM tables
+#
+connection locker;
+use mysql;
+LOCK TABLES columns_priv WRITE, db WRITE, host WRITE, user WRITE;
+FLUSH TABLES;
+--sleep 1
+#
+connection reader;
+use mysql;
+#NOTE:  This must be a multi-table select, otherwise the deadlock will not occur
+send SELECT user.Select_priv FROM user, db WHERE user.user = db.user LIMIT 1;
+--sleep 1
+#
+connection locker;
+# Make test case independent from earlier grants.
+--replace_result "Table is already up to date" "OK"
+OPTIMIZE TABLES columns_priv, db, host, user;
+UNLOCK TABLES;
+#
+connection reader;
+reap;
+use test;
+#
+connection locker;
+use test;
+#
+connection default;
+

--- 1.112/sql/sp.cc	2006-05-04 14:30:35 +02:00
+++ 1.113/sql/sp.cc	2006-05-17 22:24:32 +02:00
@@ -117,6 +117,7 @@
   TABLE_LIST tables;
   TABLE *table;
   bool not_used;
+  int refresh_not_used;
   DBUG_ENTER("open_proc_table");
 
   /*
@@ -131,7 +132,7 @@
   bzero((char*) &tables, sizeof(tables));
   tables.db= (char*) "mysql";
   tables.table_name= tables.alias= (char*)"proc";
-  if (!(table= open_table(thd, &tables, thd->mem_root, &not_used,
+  if (!(table= open_table(thd, &tables, thd->mem_root, &refresh_not_used,
                           MYSQL_LOCK_IGNORE_FLUSH)))
   {
     thd->restore_backup_open_tables_state(backup);