Merge lp:~akopytov/percona-xtrabackup/bug1170806-2.0 into lp:percona-xtrabackup/2.0

[Alexey Kopytov]

    Bug #1170806: innobackupex continuing with FTWRL even when xtrabackup
                  failed

    The problem was that when the log copying thread failed, it did not
    terminate the xtrabackup process immediately. Instead only the log
    copying thread was terminated. The main xtrabackup thread would later
    check if the 'log_copying_succeed' flag is FALSE, and terminated the
    process, but that happened later, after xtrabackup was done with copying
    InnoDB files and signaled innobackupex to lock tables and proceed with
    copying non-InnoDB files.

    The fix is to terminate the entire xtrabackup process immediately on log
    copying failure. There is no reasons to delay checking its status
    and fail much later.

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.0-param/418/

[George Ormond Lorch III (gl-az)]

Looks good except for one more general question. There seems to be a lot of exit(EXIT_FAILURE) which bypasses all resource cleanup operations. Basic OS resources (memory, open files, etc) are not a big deal as they will be cleaned up on exit, but we do use temp files and some external libraries/tools that may also have some temporary resources left behind if not de-initialized/shut down cleanly. Should we consider a new blueprint to implement a better cleanup on error?

review: Approve (g2)

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

[Alexey Kopytov (akopytov)]

Hi George,

On Wed, 24 Apr 2013 16:33:24 -0000, George Ormond Lorch III wrote:
> Review: Approve g2
>
> Looks good except for one more general question. There seems to be a lot of exit(EXIT_FAILURE) which bypasses all resource cleanup operations. Basic OS resources (memory, open files, etc) are not a big deal as they will be cleaned up on exit, but we do use temp files and some external libraries/tools that may also have some temporary resources left behind if not de-initialized/shut down cleanly. Should we consider a new blueprint to implement a better cleanup on error?
>

So files is the only concern. All temporary files should be properly
created so that they are removed automatically on process termination
(bug #1172016 was a result of a typo). And I'm not sure we should put
any effort into cleaning up non-temporary files (basically, data files).
We don't remove backup data on a failed backup currently. Even if we do
that on exit(), they will still be left behind on a crash.

[George Ormond Lorch III (gl-az)]

Yeah, I agree, I was only concerned with the 'hidden' stuff such as temps in non-obvious locations. I agree that any resulting real data is up to the user to clean.

=== modified file 'src/xtrabackup.cc'
--- src/xtrabackup.cc	2013-04-24 12:44:51 +0000
+++ src/xtrabackup.cc	2013-04-24 14:21:37 +0000
@@ -1355,7 +1355,6 @@
 lsn_t log_copy_scanned_lsn;
 ibool log_copying = TRUE;
 ibool log_copying_running = FALSE;
-ibool log_copying_succeed = FALSE;
 
 ibool xtrabackup_logfile_is_renamed = FALSE;
 
@@ -4542,17 +4541,22 @@
 
 		counter++;
 		if(counter >= SLEEPING_PERIOD * 5) {
-			if(xtrabackup_copy_logfile(log_copy_scanned_lsn, FALSE))
-				goto end;
+
+			if(xtrabackup_copy_logfile(log_copy_scanned_lsn,
+						   FALSE)) {
+				exit(EXIT_FAILURE);
+			}
+
 			counter = 0;
 		}
 	}
 
 	/* last copying */
-	if(xtrabackup_copy_logfile(log_copy_scanned_lsn, TRUE))
-		goto end;
-
-	log_copying_succeed = TRUE;
+	if(xtrabackup_copy_logfile(log_copy_scanned_lsn, TRUE)) {
+
+		exit(EXIT_FAILURE);
+	}
+
 end:
 	log_copying_running = FALSE;
 	my_thread_end();
@@ -5613,11 +5617,6 @@
 	}
 	msg("\n");
 
-	if (!log_copying_succeed) {
-		msg("xtrabackup: Error: log_copying_thread failed.\n");
-		exit(EXIT_FAILURE);
-	}
-
 	/* Signal innobackupex that log copying has stopped and it may now
 	unlock tables, so we can possibly stream xtrabackup_logfile later
 	without holding the lock. */