Bug#7823 - FLUSH TABLES WITH READ LOCK + INSERT DELAYED = deadlock

Added protection against global read lock while creating and
initializing a delayed insert handler.
Allowed to ignore a global read lock when locking the table
inside the delayed insert handler.
Added some minor improvements.


sql/lock.cc:
  Bug#7823 - FLUSH TABLES WITH READ LOCK + INSERT DELAYED = deadlock
  Changed mysql_lock_tables() to allow for ignoring global read lock.
  Added functions to set/unset protection against global read lock.
sql/mysql_priv.h:
  Bug#7823 - FLUSH TABLES WITH READ LOCK + INSERT DELAYED = deadlock
  Changed existing and added new function declarations.
sql/sql_insert.cc:
  Bug#7823 - FLUSH TABLES WITH READ LOCK + INSERT DELAYED = deadlock
  Added and extended some comments.
  Added a protection against global read lock while a handler is
  created and initialized.
  Moved the unlock of the delayed insert object past its last usage
  in delayed_get_table().
  Changed the table locking in handle_delayed_insert() so that it
  does not wait for global read lock.

sql/lock.cc

@@ -79,7 +79,8 @@ static int unlock_external(THD *thd, TABLE **table,uint count);
 static void print_lock_error(int error);
 
 
-MYSQL_LOCK *mysql_lock_tables(THD *thd,TABLE **tables,uint count)
+MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
+                              bool ignore_global_read_lock)
 {
   MYSQL_LOCK *sql_lock;
   TABLE *write_lock_used;
@@ -90,7 +91,7 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd,TABLE **tables,uint count)
     if (!(sql_lock = get_lock_data(thd,tables,count, 0,&write_lock_used)))
       break;
 
-    if (global_read_lock && write_lock_used)
+    if (global_read_lock && write_lock_used && ! ignore_global_read_lock)
     {
       /*
 	Someone has issued LOCK ALL TABLES FOR READ and we want a write lock
@@ -865,3 +866,49 @@ void make_global_read_lock_block_commit(THD *thd)
   pthread_mutex_unlock(&LOCK_open);
   thd->global_read_lock= MADE_GLOBAL_READ_LOCK_BLOCK_COMMIT;
 }
+
+
+/*
+  Set protection against global read lock.
+
+  SYNOPSIS
+    set_protect_against_global_read_lock()
+    void
+
+  RETURN
+    FALSE       OK, no global read lock exists.
+    TRUE        Error, global read lock exists already.
+*/
+
+my_bool set_protect_against_global_read_lock(void)
+{
+  my_bool       global_read_lock_exists;
+
+  pthread_mutex_lock(&LOCK_open);
+  if (! (global_read_lock_exists= test(global_read_lock)))
+    protect_against_global_read_lock++;
+  pthread_mutex_unlock(&LOCK_open);
+  return global_read_lock_exists;
+}
+
+
+/*
+  Unset protection against global read lock.
+
+  SYNOPSIS
+    unset_protect_against_global_read_lock()
+    void
+
+  RETURN
+    void
+*/
+
+void unset_protect_against_global_read_lock(void)
+{
+  pthread_mutex_lock(&LOCK_open);
+  protect_against_global_read_lock--;
+  pthread_mutex_unlock(&LOCK_open);
+  pthread_cond_broadcast(&COND_refresh);
+}
+
+

sql/mysql_priv.h

@@ -766,7 +766,8 @@ extern pthread_t signal_thread;
 extern struct st_VioSSLAcceptorFd * ssl_acceptor_fd;
 #endif /* HAVE_OPENSSL */
 
-MYSQL_LOCK *mysql_lock_tables(THD *thd,TABLE **table,uint count);
+MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **table, uint count,
+                              bool ignore_global_read_lock= FALSE);
 void mysql_unlock_tables(THD *thd, MYSQL_LOCK *sql_lock);
 void mysql_unlock_read_tables(THD *thd, MYSQL_LOCK *sql_lock);
 void mysql_unlock_some_tables(THD *thd, TABLE **table,uint count);
@@ -779,6 +780,8 @@ void unlock_global_read_lock(THD *thd);
 bool wait_if_global_read_lock(THD *thd, bool abort_on_refresh, bool is_not_commit);
 void start_waiting_global_read_lock(THD *thd);
 void make_global_read_lock_block_commit(THD *thd);
+my_bool set_protect_against_global_read_lock(void);
+void unset_protect_against_global_read_lock(void);
 
 /* Lock based on name */
 int lock_and_wait_for_table_name(THD *thd, TABLE_LIST *table_list);

sql/sql_insert.cc

@@ -647,27 +647,42 @@ static TABLE *delayed_get_table(THD *thd,TABLE_LIST *table_list)
 {
   int error;
   delayed_insert *tmp;
+  TABLE *table;
   DBUG_ENTER("delayed_get_table");
 
   if (!table_list->db)
     table_list->db=thd->db;
 
-  /* no match; create a new thread to handle the table */
+  /* Find the thread which handles this table. */
   if (!(tmp=find_handler(thd,table_list)))
   {
-    /* Don't create more than max_insert_delayed_threads */
+    /*
+      No match. Create a new thread to handle the table, but
+      no more than max_insert_delayed_threads.
+    */
     if (delayed_insert_threads >= thd->variables.max_insert_delayed_threads)
       DBUG_RETURN(0);
     thd->proc_info="Creating delayed handler";
     pthread_mutex_lock(&LOCK_delayed_create);
-    if (!(tmp=find_handler(thd,table_list)))	// Was just created
+    /*
+      The first search above was done without LOCK_delayed_create.
+      Another thread might have created the handler in between. Search again.
+    */
+    if (! (tmp= find_handler(thd, table_list)))
     {
+      /*
+        Avoid that a global read lock steps in while we are creating the
+        new thread. It would block trying to open the table. Hence, the
+        DI thread and this thread would wait until after the global
+        readlock is gone. If the read lock exists already, we leave with
+        no table and then switch to non-delayed insert.
+      */
+      if (set_protect_against_global_read_lock())
+        goto err;
       if (!(tmp=new delayed_insert()))
       {
-	thd->fatal_error=1;
 	my_error(ER_OUTOFMEMORY,MYF(0),sizeof(delayed_insert));
-	pthread_mutex_unlock(&LOCK_delayed_create);
+        goto err1;
-	DBUG_RETURN(0);
       }
       pthread_mutex_lock(&LOCK_thread_count);
       thread_count++;
@@ -676,10 +691,8 @@ static TABLE *delayed_get_table(THD *thd,TABLE_LIST *table_list)
 	  !(tmp->thd.query=my_strdup(table_list->real_name,MYF(MY_WME))))
       {
 	delete tmp;
-	thd->fatal_error=1;
 	my_error(ER_OUT_OF_RESOURCES,MYF(0));
-	pthread_mutex_unlock(&LOCK_delayed_create);
+        goto err1;
-	DBUG_RETURN(0);
       }
       tmp->table_list= *table_list;			// Needed to open table
       tmp->table_list.db= tmp->thd.db;
@@ -695,10 +708,8 @@ static TABLE *delayed_get_table(THD *thd,TABLE_LIST *table_list)
 	pthread_mutex_unlock(&tmp->mutex);
 	tmp->unlock();
 	delete tmp;
-	thd->fatal_error=1;
-	pthread_mutex_unlock(&LOCK_delayed_create);
 	net_printf(&thd->net,ER_CANT_CREATE_THREAD,error);
-	DBUG_RETURN(0);
+        goto err1;
       }
 
       /* Wait until table is open */
@@ -708,6 +719,7 @@ static TABLE *delayed_get_table(THD *thd,TABLE_LIST *table_list)
 	pthread_cond_wait(&tmp->cond_client,&tmp->mutex);
       }
       pthread_mutex_unlock(&tmp->mutex);
+      unset_protect_against_global_read_lock();
       thd->proc_info="got old table";
       if (tmp->thd.killed)
       {
@@ -719,28 +731,34 @@ static TABLE *delayed_get_table(THD *thd,TABLE_LIST *table_list)
 	  thd->net.last_errno=tmp->thd.net.last_errno;
 	}
 	tmp->unlock();
-	pthread_mutex_unlock(&LOCK_delayed_create);
+	goto err;
-	DBUG_RETURN(0);				// Continue with normal insert
       }
       if (thd->killed)
       {
 	tmp->unlock();
-	pthread_mutex_unlock(&LOCK_delayed_create);
+	goto err;
-	DBUG_RETURN(0);
       }
     }
     pthread_mutex_unlock(&LOCK_delayed_create);
   }
 
   pthread_mutex_lock(&tmp->mutex);
-  TABLE *table=tmp->get_local_table(thd);
+  table= tmp->get_local_table(thd);
   pthread_mutex_unlock(&tmp->mutex);
-  tmp->unlock();
   if (table)
     thd->di=tmp;
   else if (tmp->thd.fatal_error)
     thd->fatal_error=1;
+  /* Unlock the delayed insert object after its last access. */
+  tmp->unlock();
   DBUG_RETURN((table_list->table=table));
+
+ err1:
+  thd->fatal_error= 1;
+  unset_protect_against_global_read_lock();
+ err:
+  pthread_mutex_unlock(&LOCK_delayed_create);
+  DBUG_RETURN(0); // Continue with normal insert
 }
 
 
@@ -955,6 +973,14 @@ extern "C" pthread_handler_decl(handle_delayed_insert,arg)
   thd->killed=abort_loop;
   pthread_mutex_unlock(&LOCK_thread_count);
 
+  /*
+    Wait until the client runs into pthread_cond_wait(),
+    where we free it after the table is opened and di linked in the list.
+    If we did not wait here, the client might detect the opened table
+    before it is linked to the list. It would release LOCK_delayed_create
+    and allow another thread to create another handler for the same table,
+    since it does not find one in the list.
+  */
   pthread_mutex_lock(&di->mutex);
 #if !defined( __WIN__) && !defined(OS2)	/* Win32 calls this in pthread_create */
   if (my_thread_init())
@@ -1069,8 +1095,17 @@ extern "C" pthread_handler_decl(handle_delayed_insert,arg)
 
     if (di->tables_in_use && ! thd->lock)
     {
-      /* request for new delayed insert */
+      /*
-      if (!(thd->lock=mysql_lock_tables(thd,&di->table,1)))
+        Request for new delayed insert.
+        Lock the table, but avoid to be blocked by a global read lock.
+        If we got here while a global read lock exists, then one or more
+        inserts started before the lock was requested. These are allowed
+        to complete their work before the server returns control to the
+        client which requested the global read lock. The delayed insert
+        handler will close the table and finish when the outstanding
+        inserts are done.
+      */
+      if (! (thd->lock= mysql_lock_tables(thd, &di->table, 1, TRUE)))
       {
 	di->dead=thd->killed=1;			// Fatal error
       }