Acquire global read lock (MDL_BACKUP_STMT) after share is acquired

Part of MDEV-5336 Implement LOCK FOR BACKUP

FLUSH TABLE table_names have changed slighty as we are now opening
tables before taking the MDL lock. The difference is that FLUSH TABLE
table_name will now be blocked by a table that is waiting for FTWRL.
There should not be any new deadlocks as part of this change.

The end result is still better in most cases as FTWRL is now only
waiting for write statements to end, not for read only statements and
it's not flushing tables in use from the table cache.

Share will be needed to be able to determine if table supports online
backup. Appropriate metadata lock type in BACKUP namespace will be
acquired basing on this information.

Also made pending global read lock request to be preferred victim of MDL
deadlock detector. This allows us to hide some non-fatal deadlocks and
make FTWRL less likely to break concurrent queries.

mysql-test/main/lock_multi.result

@@ -218,6 +218,7 @@ connection con2;
 unlock tables;
 connection con3;
 a
+connection con4;
 connection default;
 disconnect con5;
 disconnect con4;
@@ -247,6 +248,7 @@ flush table t2;
 connection default;
 unlock tables;
 connection con1;
+connection con2;
 #
 # LOCK TABLES .. WRITE
 #
@@ -299,7 +301,7 @@ connection default;
 alter table t1 add column j int;
 connect  insert,localhost,root,,test,,;
 connection insert;
-insert into t1 values (1,2);;
+insert into t1 values (1,2);
 connection default;
 unlock tables;
 connection flush;

mysql-test/main/lock_multi.test

@@ -392,8 +392,13 @@ let $wait_condition=
 --echo # waiting for release of read lock
 connection con4;
 --echo # would hang and later cause a deadlock
-flush tables t2;
+--send flush tables t2
 connection con1;
+let $wait_condition=
+  select count(*) = 1 from information_schema.processlist
+  where state = "Waiting for table metadata lock" and
+        info = "flush tables t2";
+--source include/wait_condition.inc
 --echo # clean up
 unlock tables;
 connection con2;
@@ -401,6 +406,8 @@ connection con2;
 unlock tables;
 connection con3;
 --reap
+connection con4;
+--reap
 connection default;
 disconnect con5;
 disconnect con4;
@@ -437,11 +444,18 @@ let $wait_condition=
 --source include/wait_condition.inc
 --echo # statement is waiting for release of read lock
 connection con2;
-flush table t2;
+--send flush table t2
 connection default;
+let $wait_condition=
+  select count(*) = 1 from information_schema.processlist
+  where state = "Waiting for table metadata lock" and
+        info = "flush table t2";
+--source include/wait_condition.inc
 unlock tables;
 connection con1;
 --reap
+connection con2;
+--reap
 
 --echo #
 --echo # LOCK TABLES .. WRITE
@@ -553,11 +567,11 @@ let $wait_condition=
   where state = "Waiting for backup lock" and
         info = "flush tables with read lock";
 --source include/wait_condition.inc
---send insert into t1 values (1,2);
+--send insert into t1 values (1,2)
 connection default;
 let $wait_condition=
   select count(*) = 1 from information_schema.processlist
-  where state = "Waiting for backup lock" and
+  where state = "Waiting for table metadata lock" and
         info = "insert into t1 values (1,2)";
 --source include/wait_condition.inc
 unlock tables;

mysql-test/main/mdl_sync.result

@@ -3082,3 +3082,72 @@ connection default;
 SET debug_sync='RESET';
 DROP TABLE t1;
 disconnect con1;
+#
+# MDEV-5336 - Implement LOCK FOR BACKUP
+#
+# Make sure deadlock detector prefers FTWRL connection as a victim
+# and FTWRL retries lock attempt. This deadlock was present before
+# MDEV-5336.
+CREATE TABLE t1(a INT) ENGINE=InnoDB;
+CREATE TABLE t2(a INT) ENGINE=InnoDB;
+BEGIN;
+SELECT * FROM t2;
+a
+#
+connect con1,localhost,root,,;
+SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting';
+LOCK TABLES t2 WRITE;
+#
+connect con2,localhost,root,,;
+SET DEBUG_SYNC='now WAIT_FOR waiting';
+SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting';
+FLUSH TABLES WITH READ LOCK;
+#
+connection default;
+SET DEBUG_SYNC='now WAIT_FOR waiting';
+INSERT INTO t1 VALUES(1);
+COMMIT;
+connection con1;
+UNLOCK TABLES;
+connection con2;
+UNLOCK TABLES;
+connection default;
+DROP TABLE t1, t2;
+SET DEBUG_SYNC='RESET';
+disconnect con1;
+disconnect con2;
+# Make sure deadlock detector prefers FTWRL connection as a victim
+# and FTWRL retries lock attempt. This deadlock was found during
+# MDEV-5336 review.
+CREATE TABLE t1(a INT) ENGINE=InnoDB;
+CREATE TABLE t2(a INT) ENGINE=InnoDB;
+BEGIN;
+INSERT INTO t2 VALUES(1);
+SET DEBUG_SYNC='after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR go';
+INSERT INTO t1 VALUES(1);
+#
+connect con1,localhost,root,,;
+SET DEBUG_SYNC='now WAIT_FOR table_opened';
+SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting';
+LOCK TABLES t1 WRITE;
+#
+connect con2,localhost,root,,;
+SET DEBUG_SYNC='now WAIT_FOR waiting';
+SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting';
+FLUSH TABLES WITH READ LOCK;
+#
+connect con3,localhost,root,,;
+SET DEBUG_SYNC='now WAIT_FOR waiting';
+SET DEBUG_SYNC='now SIGNAL go';
+connection default;
+COMMIT;
+connection con1;
+UNLOCK TABLES;
+connection con2;
+UNLOCK TABLES;
+connection default;
+DROP TABLE t1, t2;
+SET DEBUG_SYNC='RESET';
+disconnect con1;
+disconnect con2;
+disconnect con3;

mysql-test/main/mdl_sync.test

@@ -4122,6 +4122,91 @@ DROP TABLE t1;
 
 disconnect con1;
 
+
+--echo #
+--echo # MDEV-5336 - Implement LOCK FOR BACKUP
+--echo #
+
+--echo # Make sure deadlock detector prefers FTWRL connection as a victim
+--echo # and FTWRL retries lock attempt. This deadlock was present before
+--echo # MDEV-5336.
+CREATE TABLE t1(a INT) ENGINE=InnoDB;
+CREATE TABLE t2(a INT) ENGINE=InnoDB;
+BEGIN;
+SELECT * FROM t2;
+
+--echo #
+connect(con1,localhost,root,,);
+SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting';
+send LOCK TABLES t2 WRITE;
+
+--echo #
+connect(con2,localhost,root,,);
+SET DEBUG_SYNC='now WAIT_FOR waiting';
+SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting';
+send FLUSH TABLES WITH READ LOCK;
+
+--echo #
+connection default;
+SET DEBUG_SYNC='now WAIT_FOR waiting';
+INSERT INTO t1 VALUES(1);
+COMMIT;
+
+connection con1;
+reap;
+UNLOCK TABLES;
+connection con2;
+reap;
+UNLOCK TABLES;
+connection default;
+DROP TABLE t1, t2;
+SET DEBUG_SYNC='RESET';
+disconnect con1;
+disconnect con2;
+
+--echo # Make sure deadlock detector prefers FTWRL connection as a victim
+--echo # and FTWRL retries lock attempt. This deadlock was found during
+--echo # MDEV-5336 review.
+CREATE TABLE t1(a INT) ENGINE=InnoDB;
+CREATE TABLE t2(a INT) ENGINE=InnoDB;
+BEGIN;
+INSERT INTO t2 VALUES(1);
+SET DEBUG_SYNC='after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR go';
+send INSERT INTO t1 VALUES(1);
+
+--echo #
+connect(con1,localhost,root,,);
+SET DEBUG_SYNC='now WAIT_FOR table_opened';
+SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting';
+send LOCK TABLES t1 WRITE;
+
+--echo #
+connect(con2,localhost,root,,);
+SET DEBUG_SYNC='now WAIT_FOR waiting';
+SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting';
+send FLUSH TABLES WITH READ LOCK;
+
+--echo #
+connect(con3,localhost,root,,);
+SET DEBUG_SYNC='now WAIT_FOR waiting';
+SET DEBUG_SYNC='now SIGNAL go';
+
+connection default;
+reap;
+COMMIT;
+connection con1;
+reap;
+UNLOCK TABLES;
+connection con2;
+reap;
+UNLOCK TABLES;
+connection default;
+DROP TABLE t1, t2;
+SET DEBUG_SYNC='RESET';
+disconnect con1;
+disconnect con2;
+disconnect con3;
+
 # 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

@@ -952,10 +952,10 @@ bool lock_object_name(THD *thd, MDL_key::enum_mdl_namespace mdl_type,
   semi-automatic. We assume that any statement which should be blocked
   by global read lock will either open and acquires write-lock on tables
   or acquires metadata locks on objects it is going to modify. For any
-  such statement global IX metadata lock is automatically acquired for
-  its duration (in case of LOCK TABLES until end of LOCK TABLES mode).
-  And lock_global_read_lock() simply acquires global S metadata lock
-  and thus prohibits execution of statements which modify data (unless
+  such statement MDL_BACKUP_STMT metadata lock is automatically acquired
+  for its duration (in case of LOCK TABLES until end of LOCK TABLES mode).
+  And lock_global_read_lock() simply acquires MDL_BACKUP_FTWRL1 metadata
+  lock and thus prohibits execution of statements which modify data (unless
   they modify only temporary tables). If deadlock happens it is detected
   by MDL subsystem and resolved in the standard fashion (by backing-off
   metadata locks acquired so far and restarting open tables process
@@ -998,6 +998,17 @@ bool lock_object_name(THD *thd, MDL_key::enum_mdl_namespace mdl_type,
 
   If the global read lock is already taken by this thread, then nothing is done.
 
+  Concurrent thread can acquire protection against global read lock either
+  before or after it got table metadata lock. This may lead to a deadlock if
+  there is pending global read lock request. E.g.
+  t1 does DML, holds SHARED table lock, waiting for t3 (GRL protection)
+  t2 does DDL, holds GRL protection, waiting for t1 (EXCLUSIVE)
+  t3 does FTWRL, has pending GRL, waiting for t2 (GRL)
+
+  Since this is very seldom deadlock and FTWRL connection must not hold any
+  other locks, FTWRL connection is made deadlock victim and attempt to acquire
+  GRL retried.
+
   See also "Handling of global read locks" above.
 
   @param thd     Reference to thread.
@@ -1012,7 +1023,9 @@ bool Global_read_lock::lock_global_read_lock(THD *thd)
 
   if (!m_state)
   {
+    MDL_deadlock_and_lock_abort_error_handler mdl_deadlock_handler;
     MDL_request mdl_request;
+    bool result;
 
     mysql_ha_cleanup_no_free(thd);
 
@@ -1022,9 +1035,17 @@ bool Global_read_lock::lock_global_read_lock(THD *thd)
                                                  MDL_BACKUP_FTWRL2));
     mdl_request.init(MDL_key::BACKUP, "", "", MDL_BACKUP_FTWRL1, MDL_EXPLICIT);
 
-    if (thd->mdl_context.acquire_lock(&mdl_request,
-                                      thd->variables.lock_wait_timeout))
-      DBUG_RETURN(1);
+    do
+    {
+      mdl_deadlock_handler.init();
+      thd->push_internal_handler(&mdl_deadlock_handler);
+      result= thd->mdl_context.acquire_lock(&mdl_request,
+                                            thd->variables.lock_wait_timeout);
+      thd->pop_internal_handler();
+    } while (mdl_deadlock_handler.need_reopen());
+
+    if (result)
+      DBUG_RETURN(true);
 
     m_mdl_global_read_lock= mdl_request.ticket;
     m_state= GRL_ACQUIRED;

sql/mdl.cc

@@ -1015,9 +1015,14 @@ void MDL_ticket::destroy(MDL_ticket *ticket)
 
 uint MDL_ticket::get_deadlock_weight() const
 {
-  return (m_lock->key.mdl_namespace() == MDL_key::BACKUP ||
-          m_type >= MDL_SHARED_UPGRADABLE ?
-          DEADLOCK_WEIGHT_DDL : DEADLOCK_WEIGHT_DML);
+  if (m_lock->key.mdl_namespace() == MDL_key::BACKUP)
+  {
+    if (m_type == MDL_BACKUP_FTWRL1)
+      return DEADLOCK_WEIGHT_FTWRL1;
+    return DEADLOCK_WEIGHT_DDL;
+  }
+  return m_type >= MDL_SHARED_UPGRADABLE ?
+         DEADLOCK_WEIGHT_DDL : DEADLOCK_WEIGHT_DML;
 }
 
 

sql/mdl.h

@@ -591,7 +591,8 @@ class MDL_wait_for_subgraph
 
   enum enum_deadlock_weight
   {
-    DEADLOCK_WEIGHT_DML= 0,
+    DEADLOCK_WEIGHT_FTWRL1= 0,
+    DEADLOCK_WEIGHT_DML= 1,
     DEADLOCK_WEIGHT_DDL= 100
   };
   /* A helper used to determine which lock request should be aborted. */

sql/sql_base.cc

@@ -1829,59 +1829,6 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
 
   if (! (flags & MYSQL_OPEN_HAS_MDL_LOCK))
   {
-    /*
-      We are not under LOCK TABLES and going to acquire write-lock/
-      modify the base table. We need to acquire protection against
-      global read lock until end of this statement in order to have
-      this statement blocked by active FLUSH TABLES WITH READ LOCK.
-
-      We don't need to acquire this protection under LOCK TABLES as
-      such protection already acquired at LOCK TABLES time and
-      not released until UNLOCK TABLES.
-
-      We don't block statements which modify only temporary tables
-      as these tables are not preserved by any form of
-      backup which uses FLUSH TABLES WITH READ LOCK.
-
-      TODO: The fact that we sometimes acquire protection against
-            GRL only when we encounter table to be write-locked
-            slightly increases probability of deadlock.
-            This problem will be solved once Alik pushes his
-            temporary table refactoring patch and we can start
-            pre-acquiring metadata locks at the beggining of
-            open_tables() call.
-    */
-    if (table_list->mdl_request.is_write_lock_request() &&
-        ! (flags & (MYSQL_OPEN_IGNORE_GLOBAL_READ_LOCK |
-                    MYSQL_OPEN_FORCE_SHARED_MDL |
-                    MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL |
-                    MYSQL_OPEN_SKIP_SCOPED_MDL_LOCK)) &&
-        ! ot_ctx->has_protection_against_grl())
-    {
-      MDL_request protection_request;
-      MDL_deadlock_handler mdl_deadlock_handler(ot_ctx);
-
-      if (thd->global_read_lock.can_acquire_protection())
-        DBUG_RETURN(TRUE);
-
-      protection_request.init(MDL_key::BACKUP, "", "", MDL_BACKUP_STMT,
-                              MDL_STATEMENT);
-
-      /*
-        Install error handler which if possible will convert deadlock error
-        into request to back-off and restart process of opening tables.
-      */
-      thd->push_internal_handler(&mdl_deadlock_handler);
-      bool result= thd->mdl_context.acquire_lock(&protection_request,
-                                                 ot_ctx->get_timeout());
-      thd->pop_internal_handler();
-
-      if (result)
-        DBUG_RETURN(TRUE);
-
-      ot_ctx->set_has_protection_against_grl();
-    }
-
     if (open_table_get_mdl_lock(thd, ot_ctx, &table_list->mdl_request,
                                 flags, &mdl_ticket) ||
         mdl_ticket == NULL)
@@ -2091,6 +2038,71 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
     tc_add_table(thd, table);
   }
 
+
+  if (!(flags & MYSQL_OPEN_HAS_MDL_LOCK))
+  {
+    /*
+      We are not under LOCK TABLES and going to acquire write-lock/
+      modify the base table. We need to acquire protection against
+      global read lock until end of this statement in order to have
+      this statement blocked by active FLUSH TABLES WITH READ LOCK.
+
+      We don't need to acquire this protection under LOCK TABLES as
+      such protection already acquired at LOCK TABLES time and
+      not released until UNLOCK TABLES.
+
+      We don't block statements which modify only temporary tables
+      as these tables are not preserved by any form of
+      backup which uses FLUSH TABLES WITH READ LOCK.
+
+      TODO: The fact that we sometimes acquire protection against
+            GRL only when we encounter table to be write-locked
+            slightly increases probability of deadlock.
+            This problem will be solved once Alik pushes his
+            temporary table refactoring patch and we can start
+            pre-acquiring metadata locks at the beggining of
+            open_tables() call.
+    */
+    if (table_list->mdl_request.is_write_lock_request() &&
+        ! (flags & (MYSQL_OPEN_IGNORE_GLOBAL_READ_LOCK |
+                    MYSQL_OPEN_FORCE_SHARED_MDL |
+                    MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL |
+                    MYSQL_OPEN_SKIP_SCOPED_MDL_LOCK)) &&
+        ! ot_ctx->has_protection_against_grl())
+    {
+      MDL_request protection_request;
+      MDL_deadlock_handler mdl_deadlock_handler(ot_ctx);
+
+      if (thd->global_read_lock.can_acquire_protection())
+      {
+        MYSQL_UNBIND_TABLE(table->file);
+        tc_release_table(table);
+        DBUG_RETURN(TRUE);
+      }
+
+      protection_request.init(MDL_key::BACKUP, "", "", MDL_BACKUP_STMT,
+                              MDL_STATEMENT);
+
+      /*
+        Install error handler which if possible will convert deadlock error
+        into request to back-off and restart process of opening tables.
+      */
+      thd->push_internal_handler(&mdl_deadlock_handler);
+      bool result= thd->mdl_context.acquire_lock(&protection_request,
+                                                 ot_ctx->get_timeout());
+      thd->pop_internal_handler();
+
+      if (result)
+      {
+        MYSQL_UNBIND_TABLE(table->file);
+        tc_release_table(table);
+        DBUG_RETURN(TRUE);
+      }
+
+      ot_ctx->set_has_protection_against_grl();
+    }
+  }
+
   table->mdl_ticket= mdl_ticket;
 
   table->next= thd->open_tables;		/* Link into simple list */