Implementation of

https://blueprints.launchpad.net/percona-server/+spec/backup-safe-binlog-info

One inefficiency of the backup locks feature is that even though LOCK
TABLES FOR BACKUP as a light-weight FTWRL alternative does not affect
DML statements updating InnoDB tables, LOCK BINLOG FOR BACKUP does
affect them by blocking commits.

XtraBackup uses LOCK BINLOG FOR BACKUP to:

1. retrieve consistent binary log coordinates with SHOW MASTER
   STATUS. More precisely, binary log coordinates must be consistent
   with the REDO log copy and non-transactional tables. Therefore, no
   updates can be done to non-transactional tables (this is achieved by
   an active LOCK TABLES FOR BACKUP lock), and no commits can be
   performed between SHOW MASTER STATUS and finalizing the redo log
   copy, which is achieved by LOCK BINLOG FOR BACKUP.

2. retrieve consistent master connection information for a replication
   slave. More precisely, the binary log coordinates on the master as
   reported by SHOW SLAVE STATUS must be consistent with the REDO log
   copy, so LOCK BINLOG FOR BACKUP also block the I/O replication thread.

3. For a GTID-enabled PXC node, the last binary log file must be
   included into an SST snapshot. Which is a rather artificial
   limitation on the WSREP side, but still XtraBackup obeys it by
   blocking commits with LOCK BINLOG FOR BACKUP to ensure the integrity
   of the binary log file copy.

Depending on the write rate on the server, finalizing the REDO log copy
may take a long time, so blocking commits for that duration may still
affect server availability considerably.

This task is to make the necessary server-side change to make it
possible for XtraBackup to avoid LOCK BINLOG FOR BINLOG in case percona#1, when
cases percona#2 and percona#3 do not apply, i.e. when no --slave-info is requested by
the XtraBackup options and the server is not a GTID-enabled PXC node.

Lifting limitations for cases percona#2 and percona#3 is also possible, but that is
outside the scope of this task.

The idea of the optimization is that even though InnoDB provides a
transactional storage for the binary log information (i.e. current file
name and offset), it cannot be fully trusted by XtraBackup, because that
information is only updated on an InnoDB commit operation. Which means
if the last operation before LOCK TABLES FOR BACKUP was an update to a
non-transactional storage engine, and no InnoDB commits occur before the
backup is finalized by XtraBackup, the InnoDB system header will contain
stale binary log coordinates.

One way to fix that would be to force binlog coordinates update in the
InnoDB system header on each update, regardless of the involved storage
engine(s). This is what a Galera node does to ensure XID consistency
which is stored in the same way as binary log coordinates: it forces XID
update in the InnoDB system header on each TOI operation, in particular
on each non-transactional update.

Another approach is less invasive: XtraBackup blocks all
non-transactional updates with LOCK TABLES FOR BACKUP anyway, so instead
of having all non-transactional updates flush binlog coordinates to
InnoDB unconditionally, LTFB can be modified to flush (and redo-log) the
current binlog coordinates to InnoDB. In which case binlog coordinates
provided by InnoDB will be consistent with REDO log under any
circumstances.

This patch implements the latter approach.

mysql-test/r/backup_safe_binlog_info.result

@@ -0,0 +1,6 @@
+CREATE TABLE t1 (a INT) ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1);
+CREATE TABLE t2(a INT) ENGINE=MyISAM;
+INSERT INTO t2 VALUES(2);
+LOCK TABLES FOR BACKUP;
+DROP TABLE t1;

mysql-test/suite/sys_vars/r/have_backup_safe_binlog_info_basic.result

@@ -0,0 +1,11 @@
+SELECT @@GLOBAL.have_backup_safe_binlog_info="YES";
+@@GLOBAL.have_backup_safe_binlog_info="YES"
+1
+SELECT @@SESSION.have_backup_safe_binlog_info;
+ERROR HY000: Variable 'have_backup_safe_binlog_info' is a GLOBAL variable
+SHOW GLOBAL VARIABLES LIKE 'have_backup_safe_binlog_info';
+Variable_name	Value
+have_backup_safe_binlog_info	YES
+SHOW SESSION VARIABLES LIKE 'have_backup_safe_binlog_info';
+Variable_name	Value
+have_backup_safe_binlog_info	YES

mysql-test/suite/sys_vars/t/have_backup_safe_binlog_info_basic.test

@@ -0,0 +1,7 @@
+SELECT @@GLOBAL.have_backup_safe_binlog_info="YES";
+--error ER_INCORRECT_GLOBAL_LOCAL_VAR
+SELECT @@SESSION.have_backup_safe_binlog_info;
+
+SHOW GLOBAL VARIABLES LIKE 'have_backup_safe_binlog_info';
+
+SHOW SESSION VARIABLES LIKE 'have_backup_safe_binlog_info';

mysql-test/t/backup_safe_binlog_info.test

@@ -0,0 +1,33 @@
+########################################################################
+# Tests for the backup-safe binlog info feature
+########################################################################
+
+--source include/not_embedded.inc
+--source include/have_innodb.inc
+--source include/have_log_bin.inc
+
+CREATE TABLE t1 (a INT) ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1);
+
+CREATE TABLE t2(a INT) ENGINE=MyISAM;
+INSERT INTO t2 VALUES(2);
+
+--let $binlog_file= query_get_value(SHOW MASTER STATUS, File, 1)
+--let $binlog_pos= query_get_value(SHOW MASTER STATUS, Position, 1)
+
+LOCK TABLES FOR BACKUP;
+
+--exec echo "restart: --log-error=$MYSQLTEST_VARDIR/log/my_restart.err" > $MYSQLTEST_VARDIR/tmp/mysqld.1.expect
+--shutdown_server 0
+
+--enable_reconnect
+--source include/wait_until_connected_again.inc
+--disable_reconnect
+
+--let SEARCH_FILE= $MYSQLTEST_VARDIR/log/my_restart.err
+--let SEARCH_PATTERN= InnoDB: Last MySQL binlog file position 0 $binlog_pos, file name $binlog_file
+--source include/search_pattern_in_file.inc
+
+--remove_file $SEARCH_FILE
+
+DROP TABLE t1;

sql/binlog.cc

@@ -1356,7 +1356,6 @@ static int binlog_prepare(handlerton *hton, THD *thd, bool all)
 static int binlog_start_consistent_snapshot(handlerton *hton, THD *thd)
 {
   int err= 0;
-  LOG_INFO li;
   DBUG_ENTER("binlog_start_consistent_snapshot");
 
   if ((err= thd->binlog_setup_trx_data()))
@@ -7103,9 +7102,9 @@ MYSQL_BIN_LOG::finish_commit(THD *thd)
     if (thd->commit_error == THD::CE_NONE)
     {
       /*
-        Acquire a shared lock to block commits until START TRANSACTION WITH
-        CONSISTENT SNAPSHOT completes snapshot creation for all storage
-        engines. We only reach this code if binlog_order_commits=0.
+        Acquire a shared lock to block commits if an X lock has been acquired by
+        LOCK TABLES FOR BACKUP or START TRANSACTION WITH CONSISTENT SNAPSHOT. We
+        only reach this code if binlog_order_commits=0.
       */
       DBUG_ASSERT(opt_binlog_order_commits == 0);
 
@@ -7567,7 +7566,7 @@ void MYSQL_BIN_LOG::xlock(void)
     threads with each acquiring a shared lock on LOCK_consistent_snapshot.
 
     binlog_order_commits is a dynamic variable, so we have to keep track what
-    primitives should be used in unlock_for_snapshot().
+    primitives should be used in xunlock().
   */
   if (opt_binlog_order_commits)
   {

sql/handler.cc

@@ -2418,6 +2418,50 @@ int ha_start_consistent_snapshot(THD *thd)
 }
 
 
+static my_bool store_binlog_info_handlerton(THD *thd, plugin_ref plugin,
+                                            void *arg)
+{
+  handlerton *hton= plugin_data(plugin, handlerton *);
+
+  if (hton->state == SHOW_OPTION_YES &&
+      hton->store_binlog_info)
+  {
+    hton->store_binlog_info(hton, thd);
+    *((bool *)arg)= false;
+  }
+
+  return FALSE;
+}
+
+
+int ha_store_binlog_info(THD *thd)
+{
+  LOG_INFO li;
+  bool warn= true;
+
+  if (!mysql_bin_log.is_open())
+    return 0;
+
+  DBUG_ASSERT(tc_log == &mysql_bin_log);
+
+  /* Block commits to get consistent binlog coordinates */
+  tc_log->xlock();
+
+  mysql_bin_log.raw_get_current_log(&li);
+  thd->set_trans_pos(li.log_file_name, li.pos);
+
+  plugin_foreach(thd, store_binlog_info_handlerton, MYSQL_STORAGE_ENGINE_PLUGIN,
+                 &warn);
+
+  tc_log->xunlock();
+
+  if (warn)
+    push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_UNKNOWN_ERROR,
+                 "No support for storing binlog coordinates in any storage");
+  return 0;
+}
+
+
 static my_bool flush_handlerton(THD *thd, plugin_ref plugin,
                                 void *arg)
 {

sql/handler.h

@@ -891,6 +891,7 @@ struct handlerton
    int (*panic)(handlerton *hton, enum ha_panic_function flag);
    int (*start_consistent_snapshot)(handlerton *hton, THD *thd);
    int (*clone_consistent_snapshot)(handlerton *hton, THD *thd, THD *from_thd);
+   int (*store_binlog_info)(handlerton *hton, THD *thd);
    bool (*flush_logs)(handlerton *hton);
    bool (*show_status)(handlerton *hton, THD *thd, stat_print_fn *print, enum ha_stat_type stat);
    uint (*partition_flags)();
@@ -3529,6 +3530,7 @@ int ha_release_temporary_latches(THD *thd);
 
 /* transactions: interface to handlerton functions */
 int ha_start_consistent_snapshot(THD *thd);
+int ha_store_binlog_info(THD *thd);
 int ha_commit_or_rollback_by_xid(THD *thd, XID *xid, bool commit);
 int ha_commit_trans(THD *thd, bool all, bool ignore_global_read_lock= false);
 int ha_rollback_trans(THD *thd, bool all);

sql/mysqld.cc

@@ -728,6 +728,7 @@ SHOW_COMP_OPTION have_geometry, have_rtree_keys;
 SHOW_COMP_OPTION have_crypt, have_compress;
 SHOW_COMP_OPTION have_profiling;
 SHOW_COMP_OPTION have_backup_locks;
+SHOW_COMP_OPTION have_backup_safe_binlog_info;
 SHOW_COMP_OPTION have_snapshot_cloning;
 
 ulonglong opt_log_warnings_suppress= 0;
@@ -8768,6 +8769,7 @@ static int mysql_init_variables(void)
 #endif
 
   have_backup_locks= SHOW_OPTION_YES;
+  have_backup_safe_binlog_info= SHOW_OPTION_YES;
   have_snapshot_cloning= SHOW_OPTION_YES;
 
 #if defined(__WIN__)

sql/set_var.h

@@ -345,6 +345,7 @@ extern SHOW_COMP_OPTION have_crypt;
 extern SHOW_COMP_OPTION have_compress;
 extern SHOW_COMP_OPTION have_statement_timeout;
 extern SHOW_COMP_OPTION have_backup_locks;
+extern SHOW_COMP_OPTION have_backup_safe_binlog_info;
 extern SHOW_COMP_OPTION have_snapshot_cloning;
 
 /*

sql/sql_parse.cc

@@ -2514,11 +2514,13 @@ static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables)
 
   @param thd     Thread context.
 
-  @return FALSE on success, TRUE in case of error.
+  @return false on success, true in case of error.
 */
 
 static bool lock_tables_for_backup(THD *thd)
 {
+  bool res;
+
   DBUG_ENTER("lock_tables_for_backup");
 
   if (check_global_access(thd, RELOAD_ACL))
@@ -2548,7 +2550,15 @@ static bool lock_tables_for_backup(THD *thd)
     DBUG_RETURN(true);
   }
 
-  DBUG_RETURN(thd->backup_tables_lock.acquire(thd));
+  res= thd->backup_tables_lock.acquire(thd);
+
+  if (ha_store_binlog_info(thd))
+  {
+    thd->backup_tables_lock.release(thd);
+    res= true;
+  }
+
+  DBUG_RETURN(res);
 }
 
 /**

sql/sys_vars.cc

@@ -4112,6 +4112,10 @@ static Sys_var_have Sys_have_backup_locks(
        "have_backup_locks", "have_backup_locks",
        READ_ONLY GLOBAL_VAR(have_backup_locks), NO_CMD_LINE);
 
+static Sys_var_have Sys_have_backup_safe_binlog_info(
+       "have_backup_safe_binlog_info", "have_backup_safe_binlog_info",
+       READ_ONLY GLOBAL_VAR(have_backup_safe_binlog_info), NO_CMD_LINE);
+
 static Sys_var_have Sys_have_snapshot_cloning(
        "have_snapshot_cloning", "have_snapshot_cloning",
        READ_ONLY GLOBAL_VAR(have_snapshot_cloning), NO_CMD_LINE);

storage/innobase/handler/ha_innodb.cc

@@ -1134,6 +1134,15 @@ innobase_end(
 	handlerton*		hton,	/* in: Innodb handlerton */
 	ha_panic_function	type);
 
+/*****************************************************************//**
+Stores the current binlog coordinates in the trx system header. */
+static
+int
+innobase_store_binlog_info(
+/*=======================*/
+	handlerton*	hton,	/*!< in: InnoDB handlerton */
+	THD*		thd);	/*!< in: MySQL thread handle */
+
 /*****************************************************************//**
 Creates an InnoDB transaction struct for the thd if it does not yet have one.
 Starts a new InnoDB transaction if a transaction is not yet started. And
@@ -3197,6 +3206,9 @@ innobase_init(
 	innobase_hton->clone_consistent_snapshot =
 		innobase_start_trx_and_clone_read_view;
 
+	innobase_hton->store_binlog_info =
+		innobase_store_binlog_info;
+
 	innobase_hton->flush_logs = innobase_flush_logs;
 	innobase_hton->show_status = innobase_show_status;
 	innobase_hton->flags = HTON_SUPPORTS_EXTENDED_KEYS |
@@ -3897,6 +3909,35 @@ innobase_commit_low(
 	}
 }
 
+/*****************************************************************//**
+Stores the current binlog coordinates in the trx system header. */
+static
+int
+innobase_store_binlog_info(
+/*=======================*/
+	handlerton*	hton,	/*!< in: InnoDB handlerton */
+	THD*		thd)	/*!< in: MySQL thread handle */
+{
+	const char*			file_name;
+	unsigned long long 	pos;
+	mtr_t			mtr;
+
+	DBUG_ENTER("innobase_store_binlog_info");
+
+	thd_binlog_pos(thd, &file_name, &pos);
+
+	mtr_start(&mtr);
+
+	trx_sys_update_mysql_binlog_offset(file_name, pos,
+					   TRX_SYS_MYSQL_LOG_INFO, &mtr);
+
+	mtr_commit(&mtr);
+
+	innobase_flush_logs(hton);
+
+	DBUG_RETURN(0);
+}
+
 /*****************************************************************//**
 Creates an InnoDB transaction struct for the thd if it does not yet have one.
 Starts a new InnoDB transaction if a transaction is not yet started. And