bug#3565 - HANDLER and FLUSH TABLE/TABLES deadlock.

Redesigned the handler close functions so that they are usable
at different places where waiting for closing tables is done.


mysql-test/r/flush_table.result:
  bug#3565 - HANDLER and FLUSH TABLE/TABLES deadlock.
  Added the test results.
mysql-test/t/flush_table.test:
  bug#3565 - HANDLER and FLUSH TABLE/TABLES deadlock.
  Activated old test case.
  Added new test cases.
sql/mysql_priv.h:
  bug#3565 - HANDLER and FLUSH TABLE/TABLES deadlock.
  Made mysql_ha_close() more flexible.
  Removed mysql_ha_closeall(), which closed only one table despite its name.
  Added mysql_ha_close_list(), which closes the complete list or all tables and does not lock.
  Removed a duplicate declaration.
sql/sql_base.cc:
  bug#3565 - HANDLER and FLUSH TABLE/TABLES deadlock.
  Added proper close calls before some wait points to avoid deadlocks
  or infinite loops.
sql/sql_handler.cc:
  bug#3565 - HANDLER and FLUSH TABLE/TABLES deadlock.
  Redesigned the internal function find_table_ptr_by_name().
  It can now suppress locking and tells if the requested table
  has been flushed by itself.
  Extended mysql_ha_close() so that it can now suppres locking
  and error reporting. That way it can be used at more places and the
  old function mysql_ha_closeall() is now obsolete.
  Added a new function mysql_ha_close_list() which closes a whole list
  of HANDLER tables or all HANDLER tables, if the list is NULL.
  Furthermore is can close all 'old' (marked for flush) HANDLER tables.
sql/sql_table.cc:
  bug#3565 - HANDLER and FLUSH TABLE/TABLES deadlock.
  Replaced the obsolte mysql_ha_closeall() by the new mysql_ha_close().

mysql-test/r/flush_table.result

+drop table if exists t1;
+create table t1 (a int not null auto_increment primary key);
+insert into t1 values(0);
+lock table t1 read;
+flush table t1;
+check table t1;
+Table	Op	Msg_type	Msg_text
+test.t1	check	status	OK
+drop table t1;
+drop database if exists test_test;
+create database test_test;
+use test_test;
+create table t1(table_id char(20) primary key);
+insert into t1 values ('test_test.t1');
+insert into t1 values ('');
+handler t1 open;
+handler t1 read first limit 9;
+table_id
+test_test.t1
+
+create table t2(table_id char(20) primary key);
+insert into t2 values ('test_test.t2');
+insert into t2 values ('');
+handler t2 open;
+handler t2 read first limit 9;
+table_id
+test_test.t2
+
+use test;
+drop table if exists t1;
+create table t1(table_id char(20) primary key);
+insert into t1 values ('test.t1');
+insert into t1 values ('');
+handler t1 open;
+handler t1 read first limit 9;
+table_id
+test.t1
+
+use test;
+handler test.t1 read first limit 9;
+table_id
+test.t1
+
+handler test.t2 read first limit 9;
+Unknown table 't2' in HANDLER
+handler test_test.t1 read first limit 9;
+table_id
+test_test.t1
+
+handler test_test.t2 read first limit 9;
+table_id
+test_test.t2
+
+handler test_test.t1 close;
+drop table test_test.t1;
+handler test_test.t2 close;
+drop table test_test.t2;
+drop database test_test;
+use test;
+handler test.t1 close;
+drop table test.t1;
+drop table if exists t1;
+drop table if exists t2;
+create table t1(table_id char(20) primary key);
+create table t2(table_id char(20) primary key);
+insert into t1 values ('test.t1');
+insert into t1 values ('');
+insert into t2 values ('test.t2');
+insert into t2 values ('');
+handler t1 open as a1;
+handler t1 open as a2;
+handler t2 open;
+handler a1 read first limit 9;
+table_id
+test.t1
+
+handler a2 read first limit 9;
+table_id
+test.t1
+
+handler t2 read first limit 9;
+table_id
+test.t2
+
+flush tables;
+handler a1 read first limit 9;
+Unknown table 'a1' in HANDLER
+handler a2 read first limit 9;
+Unknown table 'a2' in HANDLER
+handler t2 read first limit 9;
+Unknown table 't2' in HANDLER
+handler t1 open as a1;
+handler t1 open as a2;
+handler t2 open;
+handler a1 read first limit 9;
+table_id
+test.t1
+
+handler a2 read first limit 9;
+table_id
+test.t1
+
+handler t2 read first limit 9;
+table_id
+test.t2
+
+flush table t1;
+handler a1 read first limit 9;
+Unknown table 'a1' in HANDLER
+handler a2 read first limit 9;
+Unknown table 'a2' in HANDLER
+handler t2 read first limit 9;
+table_id
+test.t2
+
+flush table t2;
+handler t2 close;
+Unknown table 't2' in HANDLER
+drop table t1;
+drop table t2;

mysql-test/t/flush_table.test

@@ -4,10 +4,111 @@
 # Test of flush table
 #
 
-#drop table if exists t1;
-#create table t1 (a int not null auto_increment primary key);
-#insert into t1 values(0);
-#lock table t1 read;
-#flush table t1;
-#check table t1;
-#drop table t1;
+drop table if exists t1;
+create table t1 (a int not null auto_increment primary key);
+insert into t1 values(0);
+lock table t1 read;
+flush table t1;
+check table t1;
+drop table t1;
+
+#
+# Check if two database names beginning the same are seen as different.
+#
+# This database begins like the usual 'test' database.
+#
+--disable_warnings
+drop database if exists test_test;
+--enable_warnings
+create database test_test;
+use test_test;
+create table t1(table_id char(20) primary key);
+insert into t1 values ('test_test.t1');
+insert into t1 values ('');
+handler t1 open;
+handler t1 read first limit 9;
+create table t2(table_id char(20) primary key);
+insert into t2 values ('test_test.t2');
+insert into t2 values ('');
+handler t2 open;
+handler t2 read first limit 9;
+#
+# This is the usual 'test' database.
+#
+use test;
+--disable_warnings
+drop table if exists t1;
+--enable_warnings
+create table t1(table_id char(20) primary key);
+insert into t1 values ('test.t1');
+insert into t1 values ('');
+handler t1 open;
+handler t1 read first limit 9;
+#
+# Check accesibility of all the tables.
+#
+use test;
+handler test.t1 read first limit 9;
+--error 1109;
+handler test.t2 read first limit 9;
+handler test_test.t1 read first limit 9;
+handler test_test.t2 read first limit 9;
+#
+# Cleanup.
+#
+handler test_test.t1 close;
+drop table test_test.t1;
+handler test_test.t2 close;
+drop table test_test.t2;
+drop database test_test;
+#
+use test;
+handler test.t1 close;
+drop table test.t1;
+
+#
+# In the following test FLUSH TABLES produces a deadlock
+# (hang forever) if the fix for bug#3565 is missing.
+#
+--disable_warnings
+drop table if exists t1;
+drop table if exists t2;
+--enable_warnings
+create table t1(table_id char(20) primary key);
+create table t2(table_id char(20) primary key);
+insert into t1 values ('test.t1');
+insert into t1 values ('');
+insert into t2 values ('test.t2');
+insert into t2 values ('');
+handler t1 open as a1;
+handler t1 open as a2;
+handler t2 open;
+handler a1 read first limit 9;
+handler a2 read first limit 9;
+handler t2 read first limit 9;
+flush tables;
+--error 1109;
+handler a1 read first limit 9;
+--error 1109;
+handler a2 read first limit 9;
+--error 1109;
+handler t2 read first limit 9;
+#
+handler t1 open as a1;
+handler t1 open as a2;
+handler t2 open;
+handler a1 read first limit 9;
+handler a2 read first limit 9;
+handler t2 read first limit 9;
+flush table t1;
+--error 1109;
+handler a1 read first limit 9;
+--error 1109;
+handler a2 read first limit 9;
+handler t2 read first limit 9;
+flush table t2;
+--error 1109;
+handler t2 close;
+drop table t1;
+drop table t2;
+

sql/mysql_priv.h

@@ -538,8 +538,9 @@ int mysqld_show(THD *thd, const char *wild, show_var_st *variables,
 
 /* sql_handler.cc */
 int mysql_ha_open(THD *thd, TABLE_LIST *tables);
-int mysql_ha_close(THD *thd, TABLE_LIST *tables, bool dont_send_ok=0);
-int mysql_ha_closeall(THD *thd, TABLE_LIST *tables);
+int mysql_ha_close(THD *thd, TABLE_LIST *tables,
+                   bool dont_send_ok=0, bool dont_lock=0, bool no_alias=0);
+int mysql_ha_close_list(THD *thd, TABLE_LIST *tables, bool flushed=0);
 int mysql_ha_read(THD *, TABLE_LIST *,enum enum_ha_read_modes,char *,
                List<Item> *,enum ha_rkey_function,Item *,ha_rows,ha_rows);
 
@@ -587,7 +588,6 @@ void free_io_cache(TABLE *entry);
 void intern_close_table(TABLE *entry);
 bool close_thread_table(THD *thd, TABLE **table_ptr);
 void close_thread_tables(THD *thd,bool locked=0);
-bool close_thread_table(THD *thd, TABLE **table_ptr);
 void close_temporary_tables(THD *thd);
 TABLE **find_temporary_table(THD *thd, const char *db, const char *table_name);
 bool close_temporary_table(THD *thd, const char *db, const char *table_name);

sql/sql_base.cc

@@ -389,6 +389,7 @@ bool close_cached_tables(THD *thd, bool if_wait_for_refresh,
     thd->proc_info="Flushing tables";
 
     close_old_data_files(thd,thd->open_tables,1,1);
+    mysql_ha_close_list(thd, tables);
     bool found=1;
     /* Wait until all threads has closed all the tables we had locked */
     DBUG_PRINT("info", ("Waiting for others threads to close their open tables"));
@@ -857,6 +858,9 @@ TABLE *open_table(THD *thd,const char *db,const char *table_name,
     DBUG_RETURN(0);
   }
 
+  /* close handler tables which are marked for flush */
+  mysql_ha_close_list(thd, (TABLE_LIST*) NULL, /*flushed*/ 1);
+
   for (table=(TABLE*) hash_search(&open_cache,(byte*) key,key_length) ;
        table && table->in_use ;
        table = (TABLE*) hash_next(&open_cache,(byte*) key,key_length))
@@ -1222,6 +1226,7 @@ bool wait_for_tables(THD *thd)
   {
     thd->some_tables_deleted=0;
     close_old_data_files(thd,thd->open_tables,0,dropping_tables != 0);
+    mysql_ha_close_list(thd, (TABLE_LIST*) NULL, /*flushed*/ 1);
     if (!table_is_used(thd->open_tables,1))
       break;
     (void) pthread_cond_wait(&COND_refresh,&LOCK_open);

sql/sql_handler.cc

@@ -44,7 +44,9 @@
   thd->handler_tables=tmp; }
 
 static TABLE **find_table_ptr_by_name(THD *thd,const char *db,
-				      const char *table_name, bool is_alias);
+                                      const char *table_name,
+                                      bool is_alias, bool dont_lock,
+                                      bool *was_flushed);
 
 int mysql_ha_open(THD *thd, TABLE_LIST *tables)
 {
@@ -66,24 +68,60 @@ int mysql_ha_open(THD *thd, TABLE_LIST *tables)
   return 0;
 }
 
-int mysql_ha_close(THD *thd, TABLE_LIST *tables, bool dont_send_ok)
+
+/*
+  Close a HANDLER table.
+
+  SYNOPSIS
+    mysql_ha_close()
+    thd                         Thread identifier.
+    tables                      A list of tables with the first entry to close.
+    dont_send_ok                Suppresses the commands' ok message and
+                                error message and error return.
+    dont_lock                   Suppresses the normal locking of LOCK_open.
+
+  DESCRIPTION
+    Though this function takes a list of tables, only the first list entry
+    will be closed. Broadcasts a COND_refresh condition.
+    If mysql_ha_close() is not called from the parser, 'dont_send_ok'
+    must be set.
+    If the caller did already lock LOCK_open, it must set 'dont_lock'.
+
+  IMPLEMENTATION
+    find_table_ptr_by_name() closes the table, if a FLUSH TABLE is outstanding.
+    It returns a NULL pointer in this case, but flags the situation in
+    'was_flushed'. In that case the normal ER_UNKNOWN_TABLE error messages
+    is suppressed.
+
+  RETURN
+    0  ok
+    -1 error
+*/
+
+int mysql_ha_close(THD *thd, TABLE_LIST *tables,
+                   bool dont_send_ok, bool dont_lock, bool no_alias)
 {
-  TABLE **ptr=find_table_ptr_by_name(thd, tables->db, tables->alias, 1);
+  TABLE         **table_ptr;
+  bool          was_flushed;
 
-  if (*ptr)
+  table_ptr= find_table_ptr_by_name(thd, tables->db, tables->alias,
+                                    !no_alias, dont_lock, &was_flushed);
+  if (*table_ptr)
   {
-    VOID(pthread_mutex_lock(&LOCK_open));
-    if (close_thread_table(thd, ptr))
+    if (!dont_lock)
+      VOID(pthread_mutex_lock(&LOCK_open));
+    if (close_thread_table(thd, table_ptr))
     {
       /* Tell threads waiting for refresh that something has happened */
       VOID(pthread_cond_broadcast(&COND_refresh));
     }
-    VOID(pthread_mutex_unlock(&LOCK_open));
+    if (!dont_lock)
+      VOID(pthread_mutex_unlock(&LOCK_open));
   }
-  else
+  else if (!was_flushed && !dont_send_ok)
   {
-    my_printf_error(ER_UNKNOWN_TABLE,ER(ER_UNKNOWN_TABLE),MYF(0),
-		    tables->alias, "HANDLER");
+    my_printf_error(ER_UNKNOWN_TABLE, ER(ER_UNKNOWN_TABLE), MYF(0),
+                    tables->alias, "HANDLER");
     return -1;
   }
   if (!dont_send_ok)
@@ -91,13 +129,64 @@ int mysql_ha_close(THD *thd, TABLE_LIST *tables, bool dont_send_ok)
   return 0;
 }
 
-int mysql_ha_closeall(THD *thd, TABLE_LIST *tables)
+
+/*
+  Close a list of HANDLER tables.
+
+  SYNOPSIS
+    mysql_ha_close_list()
+    thd                         Thread identifier.
+    tables                      The list of tables to close. If NULL,
+                                close all HANDLER tables.
+    flushed                     Close only tables which are marked flushed.
+                                Used only if tables is NULL.
+
+  DESCRIPTION
+    The list of HANDLER tables may be NULL, in which case all HANDLER
+    tables are closed. Broadcasts a COND_refresh condition, for
+    every table closed. If 'tables' is NULL and 'flushed' is set,
+    all HANDLER tables marked for flush are closed.
+    The caller must lock LOCK_open.
+
+  IMPLEMENTATION
+    find_table_ptr_by_name() closes the table, if it is marked for flush.
+    It returns a NULL pointer in this case, but flags the situation in
+    'was_flushed'. In that case the normal ER_UNKNOWN_TABLE error messages
+    is suppressed.
+
+  RETURN
+    0  ok
+*/
+
+int mysql_ha_close_list(THD *thd, TABLE_LIST *tables, bool flushed)
 {
-  TABLE **ptr=find_table_ptr_by_name(thd, tables->db, tables->real_name, 0);
-  if (*ptr && close_thread_table(thd, ptr))
+  TABLE_LIST    *tl_item;
+  TABLE         **table_ptr;
+
+  if (tables)
+  {
+    for (tl_item= tables ; tl_item; tl_item= tl_item->next)
+    {
+      mysql_ha_close(thd, tl_item, /*dont_send_ok*/ 1,
+                     /*dont_lock*/ 1, /*no_alias*/ 1);
+    }
+  }
+  else
   {
-    /* Tell threads waiting for refresh that something has happened */
-    VOID(pthread_cond_broadcast(&COND_refresh));
+    table_ptr= &(thd->handler_tables);
+    while (*table_ptr)
+    {
+      if (! flushed || ((*table_ptr)->version != refresh_version))
+      {
+        if (close_thread_table(thd, table_ptr))
+        {
+          /* Tell threads waiting for refresh that something has happened */
+          VOID(pthread_cond_broadcast(&COND_refresh));
+        }
+        continue;
+      }
+      table_ptr= &((*table_ptr)->next);
+    }
   }
   return 0;
 }
@@ -112,7 +201,10 @@ int mysql_ha_read(THD *thd, TABLE_LIST *tables,
     ha_rows select_limit,ha_rows offset_limit)
 {
   int err, keyno=-1;
-  TABLE *table=*find_table_ptr_by_name(thd, tables->db, tables->alias, 1);
+  bool was_flushed;
+  TABLE *table= *find_table_ptr_by_name(thd, tables->db, tables->alias,
+                                        /*is_alias*/ 1, /*dont_lock*/ 0,
+                                        &was_flushed);
   if (!table)
   {
     my_printf_error(ER_UNKNOWN_TABLE,ER(ER_UNKNOWN_TABLE),MYF(0),
@@ -278,36 +370,76 @@ int mysql_ha_read(THD *thd, TABLE_LIST *tables,
   return -1;
 }
 
+
+/*
+  Find a HANDLER table by name.
+
+  SYNOPSIS
+    find_table_ptr_by_name()
+    thd                         Thread identifier.
+    db                          Database (schema) name.
+    table_name                  Table name ;-).
+    is_alias                    Table name may be an alias name.
+    dont_lock                   Suppresses the normal locking of LOCK_open.
+
+  DESCRIPTION
+    Find the table 'db'.'table_name' in the list of HANDLER tables of the
+    thread 'thd'. If the table has been marked by FLUSH TABLE(S), close it,
+    flag this situation in '*was_flushed' and broadcast a COND_refresh
+    condition.
+    An empty database (schema) name matches all database (schema) names.
+    If the caller did already lock LOCK_open, it must set 'dont_lock'.
+
+  IMPLEMENTATION
+    Just in case that the table is twice in 'thd->handler_tables' (!?!),
+    the loop does not break when the table was flushed. If another table
+    by that name was found and not flushed, '*was_flushed' is cleared again,
+    since a pointer to an open HANDLER table is returned.
+
+  RETURN
+    *was_flushed                Table has been closed due to FLUSH TABLE.
+    NULL     A HANDLER Table by that name does not exist (any more).
+    != NULL  Pointer to the TABLE structure.
+*/
+
 static TABLE **find_table_ptr_by_name(THD *thd, const char *db,
-				      const char *table_name, bool is_alias)
+                                      const char *table_name,
+                                      bool is_alias, bool dont_lock,
+                                      bool *was_flushed)
 {
   int dblen;
-  TABLE **ptr;
+  TABLE **table_ptr;
 
   DBUG_ASSERT(db);
-  dblen=*db ? strlen(db)+1 : 0;
-  ptr=&(thd->handler_tables);
+  dblen= *db ? strlen(db)+1 : 0;
+  table_ptr= &(thd->handler_tables);
+  *was_flushed= FALSE;
 
-  for (TABLE *table=*ptr; table ; table=*ptr)
+  for (TABLE *table=*table_ptr; table ; table=*table_ptr)
   {
     if ((!dblen || !memcmp(table->table_cache_key, db, dblen)) &&
-        !my_strcasecmp((is_alias ? table->table_name : table->real_name),table_name))
+        !my_strcasecmp((is_alias ? table->table_name : table->real_name),
+                       table_name))
     {
       if (table->version != refresh_version)
       {
-        VOID(pthread_mutex_lock(&LOCK_open));
-        if (close_thread_table(thd, ptr))
+        if (!dont_lock)
+          VOID(pthread_mutex_lock(&LOCK_open));
+        if (close_thread_table(thd, table_ptr))
         {
           /* Tell threads waiting for refresh that something has happened */
           VOID(pthread_cond_broadcast(&COND_refresh));
         }
-        VOID(pthread_mutex_unlock(&LOCK_open));
+        if (!dont_lock)
+          VOID(pthread_mutex_unlock(&LOCK_open));
+        *was_flushed= TRUE;
         continue;
       }
+      *was_flushed= FALSE;
       break;
     }
-    ptr=&(table->next);
+    table_ptr=&(table->next);
   }
-  return ptr;
+  return table_ptr;
 }
 

sql/sql_table.cc

@@ -176,7 +176,7 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
   for (table=tables ; table ; table=table->next)
   {
     char *db=table->db;
-    mysql_ha_closeall(thd, table);
+    mysql_ha_close(thd, table, /*dont_send_ok*/ 1, /*dont_lock*/ 1);
     if (!close_temporary_table(thd, db, table->real_name))
     {
       tmp_table_deleted=1;
@@ -1230,7 +1230,7 @@ static int mysql_admin_table(THD* thd, TABLE_LIST* tables,
   if (send_fields(thd, field_list, 1))
     DBUG_RETURN(-1);
 
-  mysql_ha_closeall(thd, tables);
+  mysql_ha_close(thd, tables, /*dont_send_ok*/ 1, /*dont_lock*/ 1);
   for (table = tables; table; table = table->next)
   {
     char table_name[NAME_LEN*2+2];
@@ -1492,7 +1492,7 @@ int mysql_alter_table(THD *thd,char *new_db, char *new_name,
   }
   used_fields=create_info->used_fields;
   
-  mysql_ha_closeall(thd, table_list);
+  mysql_ha_close(thd, table_list, /*dont_send_ok*/ 1, /*dont_lock*/ 1);
   if (!(table=open_ltable(thd,table_list,TL_WRITE_ALLOW_READ)))
     DBUG_RETURN(-1);