Backport of revno: 3685

Bug #48210 FLUSH TABLES WITH READ LOCK deadlocks
           against concurrent CREATE PROCEDURE

This deadlock occured between
a) CREATE PROCEDURE (or other commands listed below)
b) FLUSH TABLES WITH READ LOCK

If the execution of them happened in the following order:
- a) opens a table (e.g. mysql.proc)
- b) locks the global read lock (or GRL)
- a) sleeps inside wait_if_global_read_lock()
- b) increases refresh_version and sleeps waiting 
     for old tables to go away

Note that a) must start waiting on the GRL before FLUSH increases
refresh_version. Otherwise a) won't wait on the GRL and instead
close its tables for reopen, allowing FLUSH to complete and thus
avoid the deadlock.

With this patch the deadlock is avoided by making CREATE PROCEDURE
acquire a protection against global read locks before it starts
executing. This means that FLUSH TABLES WITH READ LOCK will have
to wait until CREATE PROCEDURE completes before acquiring the global
read lock, thereby avoiding the deadlock.

This is implemented by introducing a new SQL command flag called
CF_PROTECT_AGAINST_GRL. Commands marked with this flag will
acquire a GRL protection in the beginning of mysql_execute_command().
This patch adds the flag to CREATE, ALTER and DROP for PROCEDURE
and FUNCTION, as well as CREATE USER, DROP USER, RENAME USER and 
REVOKE ALL. All these commands either call open_grant_tables() or
open_system_table_for_updated() which make them susceptible for
this deadlock.

The patch also adds the CF_PROTECT_AGAINST_GRL flag to a number
of commands that previously acquired GRL protection in their
respective SQLCOM case in mysql_execute_command().

Test case that checks for GRL protection for CREATE PROCEDURE
and CREATE USER added to mdl_sync.test.

mysql-test/r/mdl_sync.result

@@ -254,3 +254,40 @@ commit;
 # Switching to connection 'default'.
 # Clean-up
 drop table t1;
+#
+# Bug#48210 FLUSH TABLES WITH READ LOCK deadlocks 
+#           against concurrent CREATE PROCEDURE
+#
+# Test 1: CREATE PROCEDURE
+# Connection 1
+# Start CREATE PROCEDURE and open mysql.proc
+SET DEBUG_SYNC= 'after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR grlwait';
+CREATE PROCEDURE p1() SELECT 1;
+# Connection 2
+SET DEBUG_SYNC= 'now WAIT_FOR table_opened';
+# Check that FLUSH must wait to get the GRL
+# and let CREATE PROCEDURE continue
+SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL grlwait';
+FLUSH TABLES WITH READ LOCK;
+# Connection 1
+# Connection 2
+UNLOCK TABLES;
+# Connection 1
+DROP PROCEDURE p1;
+SET DEBUG_SYNC= 'RESET';
+# Test 2: CREATE USER
+# Start CREATE USER and open the grant tables
+SET DEBUG_SYNC= 'after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR grlwait';
+CREATE USER 'user_1@localhost';
+# Connection 2
+SET DEBUG_SYNC= 'now WAIT_FOR table_opened';
+# Check that FLUSH must wait to get the GRL
+# and let CREATE USER continue
+SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL grlwait';
+FLUSH TABLES WITH READ LOCK;
+# Connection 1
+# Connection 2
+UNLOCK TABLES;
+# Connection 1
+DROP USER 'user_1@localhost';
+SET DEBUG_SYNC= 'RESET';

mysql-test/t/mdl_sync.test

@@ -475,6 +475,75 @@ disconnect con46673;
 drop table t1;
 
 
+--echo #
+--echo # Bug#48210 FLUSH TABLES WITH READ LOCK deadlocks 
+--echo #           against concurrent CREATE PROCEDURE
+--echo #
+
+connect (con2, localhost, root);
+
+--echo # Test 1: CREATE PROCEDURE
+
+--echo # Connection 1
+connection default;
+--echo # Start CREATE PROCEDURE and open mysql.proc
+SET DEBUG_SYNC= 'after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR grlwait';
+--send CREATE PROCEDURE p1() SELECT 1
+
+--echo # Connection 2
+connection con2;
+SET DEBUG_SYNC= 'now WAIT_FOR table_opened';
+--echo # Check that FLUSH must wait to get the GRL
+--echo # and let CREATE PROCEDURE continue
+SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL grlwait';
+--send FLUSH TABLES WITH READ LOCK
+
+--echo # Connection 1
+connection default;
+--reap
+
+--echo # Connection 2
+connection con2;
+--reap
+UNLOCK TABLES;
+
+--echo # Connection 1
+connection default;
+DROP PROCEDURE p1;
+SET DEBUG_SYNC= 'RESET';
+
+--echo # Test 2: CREATE USER
+
+connection default;
+--echo # Start CREATE USER and open the grant tables
+SET DEBUG_SYNC= 'after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR grlwait';
+--send CREATE USER 'user_1@localhost'
+
+--echo # Connection 2
+connection con2;
+SET DEBUG_SYNC= 'now WAIT_FOR table_opened';
+--echo # Check that FLUSH must wait to get the GRL
+--echo # and let CREATE USER continue
+SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL grlwait';
+--send FLUSH TABLES WITH READ LOCK
+
+--echo # Connection 1
+connection default;
+--reap
+
+--echo # Connection 2
+connection con2;
+--reap
+UNLOCK TABLES;
+
+--echo # Connection 1
+connection default;
+DROP USER 'user_1@localhost';
+SET DEBUG_SYNC= 'RESET';
+
+disconnect con2;
+
+
 # Check that all connections opened by test cases in this file are really
 # gone so execution of other tests won't be affected by their presence.
 --source include/wait_until_count_sessions.inc

sql/lock.cc

@@ -74,6 +74,7 @@
 */
 
 #include "mysql_priv.h"
+#include "debug_sync.h"
 #include <hash.h>
 #include <assert.h>
 
@@ -1119,9 +1120,33 @@ bool lock_global_read_lock(THD *thd)
   if (!thd->global_read_lock)
   {
     const char *old_message;
+    const char *new_message= "Waiting to get readlock";
     (void) pthread_mutex_lock(&LOCK_global_read_lock);
+
+#if defined(ENABLED_DEBUG_SYNC)
+    /*
+      The below sync point fires if we have to wait for
+      protect_against_global_read_lock.
+
+      WARNING: Beware to use WAIT_FOR with this sync point. We hold
+      LOCK_global_read_lock here.
+
+      Call the sync point before calling enter_cond() as it does use
+      enter_cond() and exit_cond() itself if a WAIT_FOR action is
+      executed in spite of the above warning.
+
+      Pre-set proc_info so that it is available immediately after the
+      sync point sends a SIGNAL. This makes tests more reliable.
+    */
+    if (protect_against_global_read_lock)
+    {
+      thd_proc_info(thd, new_message);
+      DEBUG_SYNC(thd, "wait_lock_global_read_lock");
+    }
+#endif /* defined(ENABLED_DEBUG_SYNC) */
+
     old_message=thd->enter_cond(&COND_global_read_lock, &LOCK_global_read_lock,
-                                "Waiting to get readlock");
+                                new_message);
     DBUG_PRINT("info",
 	       ("waiting_for: %d  protect_against: %d",
 		waiting_for_read_lock, protect_against_global_read_lock));

sql/sql_class.h

@@ -3341,6 +3341,16 @@ class select_dumpvar :public select_result_interceptor {
 */
 #define CF_DIAGNOSTIC_STMT        (1U << 8)
 
+/**
+  SQL statements that must be protected against impending global read lock
+  to prevent deadlock. This deadlock could otherwise happen if the statement
+  starts waiting for the GRL to go away inside mysql_lock_tables while at the
+  same time having "old" opened tables. The thread holding the GRL can be
+  waiting for these "old" opened tables to be closed, causing a deadlock
+  (FLUSH TABLES WITH READ LOCK).
+ */
+#define CF_PROTECT_AGAINST_GRL  (1U << 10)
+
 /* Bits in server_command_flags */
 
 /**

sql/sql_parse.cc

@@ -182,14 +182,15 @@ void init_update_queries(void)
   memset(sql_command_flags, 0, sizeof(sql_command_flags));
 
   sql_command_flags[SQLCOM_CREATE_TABLE]=   CF_CHANGES_DATA | CF_REEXECUTION_FRAGILE |
-                                            CF_AUTO_COMMIT_TRANS;
+                                            CF_AUTO_COMMIT_TRANS | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_CREATE_INDEX]=   CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS;
   sql_command_flags[SQLCOM_ALTER_TABLE]=    CF_CHANGES_DATA | CF_WRITE_LOGS_COMMAND |
-                                            CF_AUTO_COMMIT_TRANS;
+                                            CF_AUTO_COMMIT_TRANS | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_TRUNCATE]=       CF_CHANGES_DATA | CF_WRITE_LOGS_COMMAND |
                                             CF_AUTO_COMMIT_TRANS;
   sql_command_flags[SQLCOM_DROP_TABLE]=     CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS;
-  sql_command_flags[SQLCOM_LOAD]=           CF_CHANGES_DATA | CF_REEXECUTION_FRAGILE;
+  sql_command_flags[SQLCOM_LOAD]=           CF_CHANGES_DATA | CF_REEXECUTION_FRAGILE |
+                                            CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_CREATE_DB]=      CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS;
   sql_command_flags[SQLCOM_DROP_DB]=        CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS;
   sql_command_flags[SQLCOM_RENAME_TABLE]=   CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS;
@@ -207,17 +208,17 @@ void init_update_queries(void)
   sql_command_flags[SQLCOM_DROP_TRIGGER]=   CF_AUTO_COMMIT_TRANS;
 
   sql_command_flags[SQLCOM_UPDATE]=	    CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
-                                            CF_REEXECUTION_FRAGILE;
+                                            CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_UPDATE_MULTI]=   CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
-                                            CF_REEXECUTION_FRAGILE;
+                                            CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_INSERT]=	    CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
-                                            CF_REEXECUTION_FRAGILE;
+                                            CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_INSERT_SELECT]=  CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
-                                            CF_REEXECUTION_FRAGILE;
+                                            CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_DELETE]=         CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
-                                            CF_REEXECUTION_FRAGILE;
+                                            CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_DELETE_MULTI]=   CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
-                                            CF_REEXECUTION_FRAGILE;
+                                            CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_REPLACE]=        CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
                                             CF_REEXECUTION_FRAGILE;
   sql_command_flags[SQLCOM_REPLACE_SELECT]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
@@ -276,20 +277,21 @@ void init_update_queries(void)
                                                 CF_REEXECUTION_FRAGILE);
 
 
-  sql_command_flags[SQLCOM_CREATE_USER]=       CF_CHANGES_DATA;
-  sql_command_flags[SQLCOM_RENAME_USER]=       CF_CHANGES_DATA;
-  sql_command_flags[SQLCOM_DROP_USER]=         CF_CHANGES_DATA;
+  sql_command_flags[SQLCOM_CREATE_USER]=       CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL;
+  sql_command_flags[SQLCOM_RENAME_USER]=       CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL;
+  sql_command_flags[SQLCOM_DROP_USER]=         CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_GRANT]=             CF_CHANGES_DATA;
   sql_command_flags[SQLCOM_REVOKE]=            CF_CHANGES_DATA;
+  sql_command_flags[SQLCOM_REVOKE_ALL]=        CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_ALTER_DB]=          CF_CHANGES_DATA;
   sql_command_flags[SQLCOM_CREATE_FUNCTION]=   CF_CHANGES_DATA;
-  sql_command_flags[SQLCOM_DROP_FUNCTION]=     CF_CHANGES_DATA;
+  sql_command_flags[SQLCOM_DROP_FUNCTION]=     CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_OPTIMIZE]=          CF_CHANGES_DATA;
-  sql_command_flags[SQLCOM_CREATE_PROCEDURE]=  CF_CHANGES_DATA;
-  sql_command_flags[SQLCOM_CREATE_SPFUNCTION]= CF_CHANGES_DATA;
-  sql_command_flags[SQLCOM_DROP_PROCEDURE]=    CF_CHANGES_DATA;
-  sql_command_flags[SQLCOM_ALTER_PROCEDURE]=   CF_CHANGES_DATA;
-  sql_command_flags[SQLCOM_ALTER_FUNCTION]=    CF_CHANGES_DATA;
+  sql_command_flags[SQLCOM_CREATE_PROCEDURE]=  CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL;
+  sql_command_flags[SQLCOM_CREATE_SPFUNCTION]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL;
+  sql_command_flags[SQLCOM_DROP_PROCEDURE]=    CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL;
+  sql_command_flags[SQLCOM_ALTER_PROCEDURE]=   CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL;
+  sql_command_flags[SQLCOM_ALTER_FUNCTION]=    CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL;
   sql_command_flags[SQLCOM_INSTALL_PLUGIN]=    CF_CHANGES_DATA;
   sql_command_flags[SQLCOM_UNINSTALL_PLUGIN]=  CF_CHANGES_DATA;
 
@@ -1969,6 +1971,17 @@ mysql_execute_command(THD *thd)
       thd->mdl_context.release_all_locks();
   }
 
+  /*
+    Check if this command needs protection against the global read lock
+    to avoid deadlock. See CF_PROTECT_AGAINST_GRL.
+    start_waiting_global_read_lock() is called at the end of
+    mysql_execute_command().
+  */
+  if (((sql_command_flags[lex->sql_command] & CF_PROTECT_AGAINST_GRL) != 0) &&
+      !thd->locked_tables_mode)
+    if (wait_if_global_read_lock(thd, FALSE, TRUE))
+      goto error;
+
   switch (lex->sql_command) {
 
   case SQLCOM_SHOW_EVENTS:
@@ -2309,12 +2322,9 @@ case SQLCOM_PREPARE:
       start_waiting_global_read_lock(). We protect the normal CREATE
       TABLE in the same way. That way we avoid that a new table is
       created during a global read lock.
+      Protection against grl is covered by the CF_PROTECT_AGAINST_GRL flag.
     */
-    if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1))
-    {
-      res= 1;
-      goto end_with_restore_list;
-    }
+
 #ifdef WITH_PARTITION_STORAGE_ENGINE
     {
       partition_info *part_info= thd->lex->part_info;
@@ -2617,12 +2627,6 @@ case SQLCOM_PREPARE:
                             "INDEX DIRECTORY");
       create_info.data_file_name= create_info.index_file_name= NULL;
 
-      if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1))
-      {
-        res= 1;
-        break;
-      }
-
       thd->enable_slow_log= opt_log_slow_admin_statements;
       res= mysql_alter_table(thd, select_lex->db, lex->name.str,
                              &create_info,
@@ -2852,8 +2856,6 @@ case SQLCOM_PREPARE:
     DBUG_ASSERT(first_table == all_tables && first_table != 0);
     if (update_precheck(thd, all_tables))
       break;
-    if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1))
-      goto error;
     DBUG_ASSERT(select_lex->offset_limit == 0);
     unit->set_limit(select_lex);
     MYSQL_UPDATE_START(thd->query());
@@ -2884,15 +2886,6 @@ case SQLCOM_PREPARE:
     else
       res= 0;
 
-    /*
-      Protection might have already been risen if its a fall through
-      from the SQLCOM_UPDATE case above.
-    */
-    if (!thd->locked_tables_mode &&
-        lex->sql_command == SQLCOM_UPDATE_MULTI &&
-        wait_if_global_read_lock(thd, 0, 1))
-      goto error;
-
     res= mysql_multi_update_prepare(thd);
 
 #ifdef HAVE_REPLICATION
@@ -2992,11 +2985,6 @@ case SQLCOM_PREPARE:
     if ((res= insert_precheck(thd, all_tables)))
       break;
 
-    if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1))
-    {
-      res= 1;
-      break;
-    }
     MYSQL_INSERT_START(thd->query());
     res= mysql_insert(thd, all_tables, lex->field_list, lex->many_values,
 		      lex->update_list, lex->value_list,
@@ -3031,11 +3019,6 @@ case SQLCOM_PREPARE:
 
     unit->set_limit(select_lex);
 
-    if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1))
-    {
-      res= 1;
-      break;
-    }
     if (!(res= open_and_lock_tables(thd, all_tables)))
     {
       MYSQL_INSERT_SELECT_START(thd->query());
@@ -3113,11 +3096,6 @@ case SQLCOM_PREPARE:
     DBUG_ASSERT(select_lex->offset_limit == 0);
     unit->set_limit(select_lex);
 
-    if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1))
-    {
-      res= 1;
-      break;
-    }
     MYSQL_DELETE_START(thd->query());
     res = mysql_delete(thd, all_tables, select_lex->where,
                        &select_lex->order_list,
@@ -3133,12 +3111,6 @@ case SQLCOM_PREPARE:
       (TABLE_LIST *)thd->lex->auxiliary_table_list.first;
     multi_delete *del_result;
 
-    if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1))
-    {
-      res= 1;
-      break;
-    }
-
     if ((res= multi_delete_precheck(thd, all_tables)))
       break;
 
@@ -3277,9 +3249,6 @@ case SQLCOM_PREPARE:
     if (check_one_table_access(thd, privilege, all_tables))
       goto error;
 
-    if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1))
-      goto error;
-
     res= mysql_load(thd, lex->exchange, first_table, lex->field_list,
                     lex->update_list, lex->value_list, lex->duplicates,
                     lex->ignore, (bool) lex->local_file);