Bug#17862905: MYSQLDUMP CREATES USELESS METADATA LOCKS

phulakun · phulakun · commit 55ceedbc3feb · 2013-12-28T22:08:40.000+05:30
Problem Description:
--------------------
While taking the backup using tool "mysqldump" with option
"--single-transaction", MDL lock is acquired on each table dumped.
But these locks are not released until the backup operation is
completed. Any concurrent DDL operation on those tables will be
blocked until backup operation is completed. Moreover such blocked
DDL operations will also block other concurrent DML operations
(Since DDL has priority over DML) creating pile-up.

Note that officially mysqldump --single-transaction is documented as
not working/not supported in presence of concurrent DDL statements.
But it might work for some people in some scenarios and before 5.5,
due to absence of MDL, combination of "mysqldump --single-transaction"
and concurrent DDL didn't create pile-up of DML statements.

Analysis:
--------------------
"mysqldump" start transaction with consistent snapshot and sets
isolation level to "repeatable read" when it is used with option
"--single-transaction".  Data of all the tables is dumped using
SELECT statement as part of this transaction. MDL lock SR is
taken on all these tables and held till the transaction is 
committed or rolled back. Any other incompatible MDL lock request
on these tables will wait until timeout or "mysqldump" operation
is completed.

As result concurrent DDL operations on the tables which were dumped
will be blocked till the end of dump or timeout.

Note that once table is dumped it won't be used again by "mysqldump".
This fact and the fact that "mysqldump --single-transactions" produces
backup with validity point at the start of transaction (and also
retrieves binlog position for backup at the start of transaction) means
that "mysqldump --single-transaction" can release MDL on tables which
it has already dumped without introducing more problems with
consistency.

Fix:
--------------------
To make "mysqldump --single-transaction" to release locks once
dumping of the table is done, modified mysqldump client code
to dump table data after setting a savepoint. Once dumping of
table data is over, added code to rollback to the savepoint set.
Rolling back to savepoint will release MDL lock acquired for the
table.

But as of now, on rollback to savepoint, MDL locks are
not released if binlog is on. This logic is added to avoid 
dropping of tables before rollback to savepoint event is 
written to binlog. But when binlog handlerton can clear cache
and can safely rollback to savepoint without writing an event for
rollback to savepoint then also we are not releasing the MDL
locks. 

This is fixed by introducing a new handlerton function call
savepoint_rollback_can_release_mdl. We call this function to
check with each storage engine participating in transaction
whether it is safe to release MDL after rollback to savepoint.
Metadata locks are released only if all the storage engines
agreed that it is a safe thing to do.

1) For InnoDB storage engine this handlerton function can allow
   release of MDL locks if transaction has not acquired any InnoDB
   locks.

2) For Binlog this handlerton function can allow release of MDL
   locks if rollback to savepoint will completely remove any
   traces of transaction from cache.

3) Absence of this method for any storage engine means it is not
   safe to release MDL locks.

Note that this patch doesn't make "mysqldump --single-transaction"
safe in general case in presence of concurrent DDL. Nor makes it
officially supported in this case. It just allows to avoid problem
with unnecessary concurrent DDL blocking and associated DML query 
pile-up in some specific cases when it might work.
diff --git a/client/mysqldump.c b/client/mysqldump.c
@@ -4421,6 +4421,12 @@ static int dump_all_tables_in_db(char *database)
     else
       verbose_msg("-- dump_all_tables_in_db : logs flushed successfully!\n");
   }
+  if (opt_single_transaction && mysql_get_server_version(mysql) >= 50500)
+  {
+    verbose_msg("-- Setting savepoint...\n");
+    if (mysql_query_with_error_report(mysql, 0, "SAVEPOINT sp"))
+      DBUG_RETURN(1);
+  }
   while ((table= getTableName(0)))
   {
     char *end= strmov(afterdot, table);
@@ -4438,6 +4444,23 @@ static int dump_all_tables_in_db(char *database)
           maybe_exit(EX_MYSQLERR);
         }
       }
+
+      /**
+        ROLLBACK TO SAVEPOINT in --single-transaction mode to release metadata
+        lock on table which was already dumped. This allows to avoid blocking
+        concurrent DDL on this table without sacrificing correctness, as we
+        won't access table second time and dumps created by --single-transaction
+        mode have validity point at the start of transaction anyway.
+        Note that this doesn't make --single-transaction mode with concurrent
+        DDL safe in general case. It just improves situation for people for whom
+        it might be working.
+      */
+      if (opt_single_transaction && mysql_get_server_version(mysql) >= 50500)
+      {
+        verbose_msg("-- Rolling back to savepoint sp...\n");
+        if (mysql_query_with_error_report(mysql, 0, "ROLLBACK TO SAVEPOINT sp"))
+          maybe_exit(EX_MYSQLERR);
+      }
     }
     else
     {
@@ -4460,6 +4483,14 @@ static int dump_all_tables_in_db(char *database)
       }
     }
   }
+
+  if (opt_single_transaction && mysql_get_server_version(mysql) >= 50500)
+  {
+    verbose_msg("-- Releasing savepoint...\n");
+    if (mysql_query_with_error_report(mysql, 0, "RELEASE SAVEPOINT sp"))
+      DBUG_RETURN(1);
+  }
+
   if (opt_events && mysql_get_server_version(mysql) >= 50106)
   {
     DBUG_PRINT("info", ("Dumping events for database %s", database));
@@ -4702,6 +4733,13 @@ static int dump_selected_tables(char *db, char **table_names, int tables)
   if (opt_xml)
     print_xml_tag(md_result_file, "", "\n", "database", "name=", db, NullS);
 
+  if (opt_single_transaction && mysql_get_server_version(mysql) >= 50500)
+  {
+    verbose_msg("-- Setting savepoint...\n");
+    if (mysql_query_with_error_report(mysql, 0, "SAVEPOINT sp"))
+      DBUG_RETURN(1);
+  }
+
   /* Dump each selected table */
   for (pos= dump_tables; pos < end; pos++)
   {
@@ -4717,6 +4755,31 @@ static int dump_selected_tables(char *db, char **table_names, int tables)
         maybe_exit(EX_MYSQLERR);
       }
     }
+
+    /**
+      ROLLBACK TO SAVEPOINT in --single-transaction mode to release metadata
+      lock on table which was already dumped. This allows to avoid blocking
+      concurrent DDL on this table without sacrificing correctness, as we
+      won't access table second time and dumps created by --single-transaction
+      mode have validity point at the start of transaction anyway.
+      Note that this doesn't make --single-transaction mode with concurrent
+      DDL safe in general case. It just improves situation for people for whom
+      it might be working.
+    */
+    if (opt_single_transaction && mysql_get_server_version(mysql) >= 50500)
+    {
+      verbose_msg("-- Rolling back to savepoint sp...\n");
+      if (mysql_query_with_error_report(mysql, 0, "ROLLBACK TO SAVEPOINT sp"))
+        maybe_exit(EX_MYSQLERR);
+    }
+  }
+
+  if (opt_single_transaction && mysql_get_server_version(mysql) >= 50500)
+  {
+    verbose_msg("-- Releasing savepoint...\n");
+    if (mysql_query_with_error_report(mysql, 0, "RELEASE SAVEPOINT sp"))
+      DBUG_RETURN(1);
+
   }
 
   /* Dump each selected view */
diff --git a/mysql-test/r/mysqldump.result b/mysql-test/r/mysqldump.result
@@ -5315,12 +5315,16 @@ UNLOCK TABLES;
 -- Connecting to localhost...
 -- main : logs flushed successfully!
 -- Starting transaction...
+-- Setting savepoint...
 -- Retrieving table structure for table t1...
 -- Sending SELECT query...
 -- Retrieving rows...
+-- Rolling back to savepoint sp...
 -- Retrieving table structure for table t2...
 -- Sending SELECT query...
 -- Retrieving rows...
+-- Rolling back to savepoint sp...
+-- Releasing savepoint...
 -- Disconnecting from localhost...
 
 #### Dump ends here ####
diff --git a/sql/binlog.cc b/sql/binlog.cc
@@ -73,6 +73,8 @@ static int binlog_start_trans_and_stmt(THD *thd, Log_event *start_event);
 static int binlog_close_connection(handlerton *hton, THD *thd);
 static int binlog_savepoint_set(handlerton *hton, THD *thd, void *sv);
 static int binlog_savepoint_rollback(handlerton *hton, THD *thd, void *sv);
+static bool binlog_savepoint_rollback_can_release_mdl(handlerton *hton,
+                                                      THD *thd);
 static int binlog_commit(handlerton *hton, THD *thd, bool all);
 static int binlog_rollback(handlerton *hton, THD *thd, bool all);
 static int binlog_prepare(handlerton *hton, THD *thd, bool all);
@@ -881,6 +883,8 @@ static int binlog_init(void *p)
   binlog_hton->close_connection= binlog_close_connection;
   binlog_hton->savepoint_set= binlog_savepoint_set;
   binlog_hton->savepoint_rollback= binlog_savepoint_rollback;
+  binlog_hton->savepoint_rollback_can_release_mdl=
+                                     binlog_savepoint_rollback_can_release_mdl;
   binlog_hton->commit= binlog_commit;
   binlog_hton->rollback= binlog_rollback;
   binlog_hton->prepare= binlog_prepare;
@@ -1760,6 +1764,29 @@ static int binlog_savepoint_rollback(handlerton *hton, THD *thd, void *sv)
   DBUG_RETURN(0);
 }
 
+/**
+  Check whether binlog state allows to safely release MDL locks after
+  rollback to savepoint.
+
+  @param hton  The binlog handlerton.
+  @param thd   The client thread that executes the transaction.
+
+  @return true  - It is safe to release MDL locks.
+          false - If it is not.
+*/
+static bool binlog_savepoint_rollback_can_release_mdl(handlerton *hton,
+                                                      THD *thd)
+{
+  DBUG_ENTER("binlog_savepoint_rollback_can_release_mdl");
+  /*
+    If we have not updated any non-transactional tables rollback
+    to savepoint will simply truncate binlog cache starting from
+    SAVEPOINT command. So it should be safe to release MDL acquired
+    after SAVEPOINT command in this case.
+  */
+  DBUG_RETURN(!trans_cannot_safely_rollback(thd));
+}
+
 #ifdef HAVE_REPLICATION
 
 /*
diff --git a/sql/handler.cc b/sql/handler.cc
@@ -1993,6 +1993,41 @@ int ha_release_temporary_latches(THD *thd)
   return 0;
 }
 
+/**
+  Check if all storage engines used in transaction agree that after
+  rollback to savepoint it is safe to release MDL locks acquired after
+  savepoint creation.
+
+  @param thd   The client thread that executes the transaction.
+
+  @return true  - It is safe to release MDL locks.
+          false - If it is not.
+*/
+bool ha_rollback_to_savepoint_can_release_mdl(THD *thd)
+{
+  Ha_trx_info *ha_info;
+  THD_TRANS *trans= (thd->in_sub_stmt ? &thd->transaction.stmt :
+                                        &thd->transaction.all);
+
+  DBUG_ENTER("ha_rollback_to_savepoint_can_release_mdl");
+
+  /**
+    Checking whether it is safe to release metadata locks after rollback to
+    savepoint in all the storage engines that are part of the transaction.
+  */
+  for (ha_info= trans->ha_list; ha_info; ha_info= ha_info->next())
+  {
+    handlerton *ht= ha_info->ht();
+    DBUG_ASSERT(ht);
+
+    if (ht->savepoint_rollback_can_release_mdl == 0 ||
+        ht->savepoint_rollback_can_release_mdl(ht, thd) == false)
+      DBUG_RETURN(false);
+  }
+
+  DBUG_RETURN(true);
+}
+
 int ha_rollback_to_savepoint(THD *thd, SAVEPOINT *sv)
 {
   int error=0;
diff --git a/sql/handler.h b/sql/handler.h
@@ -850,6 +850,13 @@ struct handlerton
      to the savepoint_set call
    */
    int  (*savepoint_rollback)(handlerton *hton, THD *thd, void *sv);
+   /**
+     Check if storage engine allows to release metadata locks which were
+     acquired after the savepoint if rollback to savepoint is done.
+     @return true  - If it is safe to release MDL locks.
+             false - If it is not.
+   */
+   bool (*savepoint_rollback_can_release_mdl)(handlerton *hton, THD *thd);
    int  (*savepoint_release)(handlerton *hton, THD *thd, void *sv);
    /*
      'all' is true if it's a real commit, that makes persistent changes
@@ -3423,6 +3430,7 @@ int ha_enable_transaction(THD *thd, bool on);
 
 /* savepoints */
 int ha_rollback_to_savepoint(THD *thd, SAVEPOINT *sv);
+bool ha_rollback_to_savepoint_can_release_mdl(THD *thd);
 int ha_savepoint(THD *thd, SAVEPOINT *sv);
 int ha_release_savepoint(THD *thd, SAVEPOINT *sv);
 
diff --git a/sql/transaction.cc b/sql/transaction.cc
@@ -630,21 +630,40 @@ bool trans_rollback_to_savepoint(THD *thd, LEX_STRING name)
     DBUG_RETURN(TRUE);
   }
 
+  /**
+    Checking whether it is safe to release metadata locks acquired after
+    savepoint, if rollback to savepoint is successful.
+  
+    Whether it is safe to release MDL after rollback to savepoint depends
+    on storage engines participating in transaction:
+  
+    - InnoDB doesn't release any row-locks on rollback to savepoint so it
+      is probably a bad idea to release MDL as well.
+    - Binary log implementation in some cases (e.g when non-transactional
+      tables involved) may choose not to remove events added after savepoint
+      from transactional cache, but instead will write them to binary
+      log accompanied with ROLLBACK TO SAVEPOINT statement. Since the real
+      write happens at the end of transaction releasing MDL on tables
+      mentioned in these events (i.e. acquired after savepoint and before
+      rollback ot it) can break replication, as concurrent DROP TABLES
+      statements will be able to drop these tables before events will get
+      into binary log,
+  
+    For backward-compatibility reasons we always release MDL if binary
+    logging is off.
+  */
+  bool mdl_can_safely_rollback_to_savepoint=
+                (!(mysql_bin_log.is_open() && thd->variables.sql_log_bin) ||
+                 ha_rollback_to_savepoint_can_release_mdl(thd));
+
   if (ha_rollback_to_savepoint(thd, sv))
     res= TRUE;
   else if (thd->transaction.all.cannot_safely_rollback() && !thd->slave_thread)
     thd->transaction.push_unsafe_rollback_warnings(thd);
 
   thd->transaction.savepoints= sv;
 
-  /*
-    Release metadata locks that were acquired during this savepoint unit
-    unless binlogging is on. Releasing locks with binlogging on can break
-    replication as it allows other connections to drop these tables before
-    rollback to savepoint is written to the binlog.
-  */
-  bool binlog_on= mysql_bin_log.is_open() && thd->variables.sql_log_bin;
-  if (!res && !binlog_on)
+  if (!res && mdl_can_safely_rollback_to_savepoint)
     thd->mdl_context.rollback_to_savepoint(sv->mdl_savepoint);
 
   DBUG_RETURN(MY_TEST(res));
diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc
@@ -733,6 +733,19 @@ innobase_rollback_to_savepoint(
 					be rolled back to savepoint */
 	void*		savepoint);	/*!< in: savepoint data */
 
+/*****************************************************************//**
+Check whether innodb state allows to safely release MDL locks after
+rollback to savepoint.
+@return true if it is safe, false if its not safe. */
+static
+bool
+innobase_rollback_to_savepoint_can_release_mdl(
+/*===========================================*/
+	handlerton*	hton,		/*!< in/out: InnoDB handlerton */
+	THD*		thd);		/*!< in: handle to the MySQL thread of
+					the user whose XA transaction should
+					be rolled back to savepoint */
+
 /*****************************************************************//**
 Sets a transaction savepoint.
 @return	always 0, that is, always succeeds */
@@ -2778,6 +2791,8 @@ innobase_init(
 	innobase_hton->close_connection = innobase_close_connection;
 	innobase_hton->savepoint_set = innobase_savepoint;
 	innobase_hton->savepoint_rollback = innobase_rollback_to_savepoint;
+	innobase_hton->savepoint_rollback_can_release_mdl =
+				innobase_rollback_to_savepoint_can_release_mdl;
 	innobase_hton->savepoint_release = innobase_release_savepoint;
 	innobase_hton->commit = innobase_commit;
 	innobase_hton->rollback = innobase_rollback;
@@ -3695,6 +3710,38 @@ innobase_rollback_to_savepoint(
 	DBUG_RETURN(convert_error_code_to_mysql(error, 0, NULL));
 }
 
+/*****************************************************************//**
+Check whether innodb state allows to safely release MDL locks after
+rollback to savepoint. 
+When binlog is on, MDL locks acquired after savepoint unit are not 
+released if there are any locks held in InnoDB.
+@return true if it is safe, false if its not safe. */
+static
+bool
+innobase_rollback_to_savepoint_can_release_mdl(
+/*===========================================*/
+	handlerton*	hton,		/*!< in: InnoDB handlerton */
+	THD*		thd)		/*!< in: handle to the MySQL thread
+					of the user whose transaction should
+					be rolled back to savepoint */
+{
+	trx_t*		trx;
+
+	DBUG_ENTER("innobase_rollback_to_savepoint_can_release_mdl");
+	DBUG_ASSERT(hton == innodb_hton_ptr);
+
+	trx = check_trx_exists(thd);
+	ut_ad(trx);
+
+        /* If transaction has not acquired any locks then it is safe
+	   to release MDL after rollback to savepoint */
+	if (!(UT_LIST_GET_LEN(trx->lock.trx_locks))) {
+		DBUG_RETURN(true);
+	}
+
+	DBUG_RETURN(false);
+}
+
 /*****************************************************************//**
 Release transaction savepoint name.
 @return 0 if success, HA_ERR_NO_SAVEPOINT if no savepoint with the
