MDEV-13575 On failure, Mariabackup --backup --safe-slave-backup may forget to START SLAVE SQL_THREAD

backup_release(): New function, refactored from backup_finish().
Release some resources that may have been acquired by backup_startup()
and should be released even after a failed operation.

xtrabackup_backup_low(): Refactored from xtrabackup_backup_func().

xtrabackup_backup_func(): Always call backup_release() after calling
backup_start().

Author: dr-m

extra/mariabackup/backup_copy.cc

@@ -1342,8 +1342,8 @@ backup_files(const char *from, bool prep_mode)
 	return(ret);
 }
 
-bool
-backup_start()
+/** Start --backup */
+bool backup_start()
 {
 	if (!opt_no_lock) {
 		if (opt_safe_slave_backup) {
@@ -1418,9 +1418,8 @@ backup_start()
 	return(true);
 }
 
-
-bool
-backup_finish()
+/** Release resources after backup_start() */
+void backup_release()
 {
 	/* release all locks */
 	if (!opt_no_lock) {
@@ -1435,7 +1434,11 @@ backup_finish()
 		xb_mysql_query(mysql_connection,
 				"START SLAVE SQL_THREAD", false);
 	}
+}
 
+/** Finish after backup_start() and backup_release() */
+bool backup_finish()
+{
 	/* Copy buffer pool dump or LRU dump */
 	if (!opt_rsync) {
 		if (buffer_pool_filename && file_exists(buffer_pool_filename)) {

extra/mariabackup/backup_copy.h

@@ -31,10 +31,12 @@ copy_file(ds_ctxt_t *datasink,
 	  const char *dst_file_path,
 	  uint thread_n);
 
-bool
-backup_start();
-bool
-backup_finish();
+/** Start --backup */
+bool backup_start();
+/** Release resources after backup_start() */
+void backup_release();
+/** Finish after backup_start() and backup_release() */
+bool backup_finish();
 bool
 apply_log_finish();
 bool

extra/mariabackup/xtrabackup.cc

@@ -3326,14 +3326,81 @@ static void stop_backup_threads()
 	}
 }
 
+/** Implement the core of --backup
+@return	whether the operation succeeded */
+static
+bool
+xtrabackup_backup_low()
+{
+	/* read the latest checkpoint lsn */
+	{
+		ulint	max_cp_field;
+
+		log_mutex_enter();
+
+		if (recv_find_max_checkpoint(&max_cp_field) == DB_SUCCESS
+		    && log_sys->log.format != 0) {
+			metadata_to_lsn = mach_read_from_8(
+				log_sys->checkpoint_buf + LOG_CHECKPOINT_LSN);
+			msg("xtrabackup: The latest check point"
+			    " (for incremental): '" LSN_PF "'\n",
+			    metadata_to_lsn);
+		} else {
+			metadata_to_lsn = 0;
+			msg("xtrabackup: Error: recv_find_max_checkpoint() failed.\n");
+		}
+		log_mutex_exit();
+	}
+
+	stop_backup_threads();
+
+	if (!dst_log_file || xtrabackup_copy_logfile(COPY_LAST)) {
+		return false;
+	}
+
+	if (ds_close(dst_log_file)) {
+		dst_log_file = NULL;
+		return false;
+	}
+
+	dst_log_file = NULL;
+
+	if(!xtrabackup_incremental) {
+		strcpy(metadata_type, "full-backuped");
+		metadata_from_lsn = 0;
+	} else {
+		strcpy(metadata_type, "incremental");
+		metadata_from_lsn = incremental_lsn;
+	}
+	metadata_last_lsn = log_copy_scanned_lsn;
+
+	if (!xtrabackup_stream_metadata(ds_meta)) {
+		msg("xtrabackup: Error: failed to stream metadata.\n");
+		return false;
+	}
+	if (xtrabackup_extra_lsndir) {
+		char	filename[FN_REFLEN];
+
+		sprintf(filename, "%s/%s", xtrabackup_extra_lsndir,
+			XTRABACKUP_METADATA_FILENAME);
+		if (!xtrabackup_write_metadata(filename)) {
+			msg("xtrabackup: Error: failed to write metadata "
+			    "to '%s'.\n", filename);
+			return false;
+		}
+
+	}
+
+	return true;
+}
+
 /** Implement --backup
 @return	whether the operation succeeded */
 static
 bool
 xtrabackup_backup_func()
 {
 	MY_STAT			 stat_info;
-	lsn_t			 latest_cp;
 	uint			 i;
 	uint			 count;
 	pthread_mutex_t		 count_mutex;
@@ -3733,70 +3800,19 @@ xtrabackup_backup_func()
 	}
 	}
 
-	if (!backup_start()) {
-		goto fail;
-	}
-
-	/* read the latest checkpoint lsn */
-	{
-		ulint	max_cp_field;
-
-		log_mutex_enter();
-
-		if (recv_find_max_checkpoint(&max_cp_field) == DB_SUCCESS
-		    && log_sys->log.format != 0) {
-			latest_cp = mach_read_from_8(log_sys->checkpoint_buf +
-						     LOG_CHECKPOINT_LSN);
-			msg("xtrabackup: The latest check point"
-			    " (for incremental): '" LSN_PF "'\n", latest_cp);
-		} else {
-			latest_cp = 0;
-			msg("xtrabackup: Error: recv_find_max_checkpoint() failed.\n");
-		}
-		log_mutex_exit();
-	}
-
-	stop_backup_threads();
-
-	if (!dst_log_file || xtrabackup_copy_logfile(COPY_LAST)) {
-		goto fail;
-	}
+	bool ok = backup_start();
 
-	if (ds_close(dst_log_file)) {
-		dst_log_file = NULL;
-		goto fail;
-	}
-
-	dst_log_file = NULL;
+	if (ok) {
+		ok = xtrabackup_backup_low();
 
-	if(!xtrabackup_incremental) {
-		strcpy(metadata_type, "full-backuped");
-		metadata_from_lsn = 0;
-	} else {
-		strcpy(metadata_type, "incremental");
-		metadata_from_lsn = incremental_lsn;
-	}
-	metadata_to_lsn = latest_cp;
-	metadata_last_lsn = log_copy_scanned_lsn;
+		backup_release();
 
-	if (!xtrabackup_stream_metadata(ds_meta)) {
-		msg("xtrabackup: Error: failed to stream metadata.\n");
-		goto fail;
-	}
-	if (xtrabackup_extra_lsndir) {
-		char	filename[FN_REFLEN];
-
-		sprintf(filename, "%s/%s", xtrabackup_extra_lsndir,
-			XTRABACKUP_METADATA_FILENAME);
-		if (!xtrabackup_write_metadata(filename)) {
-			msg("xtrabackup: Error: failed to write metadata "
-			    "to '%s'.\n", filename);
-			goto fail;
+		if (ok) {
+			backup_finish();
 		}
-
 	}
 
-	if (!backup_finish()) {
+	if (!ok) {
 		goto fail;
 	}
 
@@ -3809,10 +3825,10 @@ xtrabackup_backup_func()
 	xb_data_files_close();
 
 	/* Make sure that the latest checkpoint was included */
-	if (latest_cp > log_copy_scanned_lsn) {
+	if (metadata_to_lsn > log_copy_scanned_lsn) {
 		msg("xtrabackup: error: failed to copy enough redo log ("
 		    "LSN=" LSN_PF "; checkpoint LSN=" LSN_PF ").\n",
-		    log_copy_scanned_lsn, latest_cp);
+		    log_copy_scanned_lsn, metadata_to_lsn);
 		goto fail;
 	}