merge MySQL-5.6 bugfix "Bug#17862905: MYSQLDUMP CREATES USELESS METADATA LOCKS"

revno: 5716
committer: Praveenkumar Hulakund <praveenkumar.hulakund@oracle.com>
branch nick: mysql_5_6
timestamp: Sat 2013-12-28 22:08:40 +0530
message:
  Bug#17862905: MYSQLDUMP CREATES USELESS METADATA LOCKS

6 files changed, 192 insertions, 27 deletions

diff --git a/client/mysqldump.c b/client/mysqldump.c
index a0222f370b3..b3a679261d7 100644
--- a/client/mysqldump.c
+++ b/client/mysqldump.c
@@ -4406,6 +4406,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);
@@ -4423,6 +4429,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
     {
@@ -4445,6 +4468,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));
@@ -4687,6 +4718,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++)
   {
@@ -4702,6 +4740,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
index c67a7a00425..9dedbd1d133 100644
--- a/mysql-test/r/mysqldump.result
+++ b/mysql-test/r/mysqldump.result
@@ -5206,12 +5206,16 @@ INSERT INTO b12809202_db.t2 VALUES (1), (2), (3);
 -- 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/handler.cc b/sql/handler.cc
index 50044cf3cab..3cb2106a443 100644
--- a/sql/handler.cc
+++ b/sql/handler.cc
@@ -1978,6 +1978,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
index 13b783b964b..0b8bd6e9ce6 100644
--- a/sql/handler.h
+++ b/sql/handler.h
@@ -1025,6 +1025,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
@@ -4046,6 +4053,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/log.cc b/sql/log.cc
index 901e4e5041d..dafe89ac7f0 100644
--- a/sql/log.cc
+++ b/sql/log.cc
@@ -73,6 +73,8 @@ static int binlog_init(void *p);
 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);
@@ -1629,6 +1631,8 @@ 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;
@@ -1879,6 +1883,32 @@ static int binlog_prepare(handlerton *hton, THD *thd, bool all)
   return 0;
 }
 
+/*
+  We flush the cache wrapped in a beging/rollback if:
+    . aborting a single or multi-statement transaction and;
+    . the OPTION_KEEP_LOG is active or;
+    . the format is STMT and a non-trans table was updated or;
+    . the format is MIXED and a temporary non-trans table was
+      updated or;
+    . the format is MIXED, non-trans table was updated and
+      aborting a single statement transaction;
+*/
+static bool trans_cannot_safely_rollback(THD *thd, bool all)
+{
+  binlog_cache_mngr *const cache_mngr=
+    (binlog_cache_mngr*) thd_get_ha_data(thd, binlog_hton);
+
+  return ((thd->variables.option_bits & OPTION_KEEP_LOG) ||
+          (trans_has_updated_non_trans_table(thd) &&
+           thd->variables.binlog_format == BINLOG_FORMAT_STMT) ||
+          (cache_mngr->trx_cache.changes_to_non_trans_temp_table() &&
+           thd->variables.binlog_format == BINLOG_FORMAT_MIXED) ||
+          (trans_has_updated_non_trans_table(thd) &&
+           ending_single_stmt_trans(thd,all) &&
+           thd->variables.binlog_format == BINLOG_FORMAT_MIXED));
+}
+
+
 /**
   This function is called once after each statement.
 
@@ -1999,25 +2029,7 @@ static int binlog_rollback(handlerton *hton, THD *thd, bool all)
   }
   else if (!error)
   {  
-    /*
-      We flush the cache wrapped in a beging/rollback if:
-        . aborting a single or multi-statement transaction and;
-        . the OPTION_KEEP_LOG is active or;
-        . the format is STMT and a non-trans table was updated or;
-        . the format is MIXED and a temporary non-trans table was
-          updated or;
-        . the format is MIXED, non-trans table was updated and
-          aborting a single statement transaction;
-    */
-    if (ending_trans(thd, all) &&
-        ((thd->variables.option_bits & OPTION_KEEP_LOG) ||
-         (trans_has_updated_non_trans_table(thd) &&
-          thd->variables.binlog_format == BINLOG_FORMAT_STMT) ||
-         (cache_mngr->trx_cache.changes_to_non_trans_temp_table() &&
-          thd->variables.binlog_format == BINLOG_FORMAT_MIXED) ||
-         (trans_has_updated_non_trans_table(thd) &&
-          ending_single_stmt_trans(thd,all) &&
-          thd->variables.binlog_format == BINLOG_FORMAT_MIXED)))
+    if (ending_trans(thd, all) && trans_cannot_safely_rollback(thd, all))
       error= binlog_rollback_flush_trx_cache(thd, all, cache_mngr);
     /*
       Truncate the cache if:
@@ -2197,6 +2209,30 @@ static int binlog_savepoint_rollback(handlerton *hton, THD *thd, void *sv)
 }
 
 
+/**
+  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, true));
+}
+
+
 int check_binlog_magic(IO_CACHE* log, const char** errmsg)
 {
   uchar magic[4];
diff --git a/sql/transaction.cc b/sql/transaction.cc
index 3575ff52e66..933e39ae357 100644
--- a/sql/transaction.cc
+++ b/sql/transaction.cc
@@ -574,6 +574,32 @@ 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->variables.option_bits & OPTION_KEEP_LOG) ||
@@ -585,14 +611,7 @@ bool trans_rollback_to_savepoint(THD *thd, LEX_STRING name)
 
   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));