Merge lp:~sergei.glushchenko/percona-xtrabackup/xb20-bug1038127 into lp:percona-xtrabackup/2.0

[Sergei Glushchenko]

space_id in .meta file been made optional.

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

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

[Alexey Kopytov (akopytov)]

Some comments on the fix and the test case:

  I think we should print a warning when space_id is not found in the
  metadata file, something along the lines of "This backup was taken
  with XtraBackup 2.0.1 or earlier, some DDL operations between full and
  incremental backups may be handled incorrectly".

  The if() statements in xb_read_delta_metadata() should really be "if
  () else ...". And InnoDB code uses braces around single-statement
  blocks.

  In the test case:

  - there's no need to do --copy-back, restart the server and calculate
    checksums. The test case must verify that prepare on specially
    formatted .delta files does not fail (and possibly issues a warning)
  - the "Inserting more data" step is meaningless, it doesn't affect the
    deltas, as there will be no changes in the data
  - there's no need for 'ls -al ..'
  - the construct with 'mv ...; grep ...' is equivalent to:

    sed -ie 's/space_id/ d' $topdir/inc/test/t1.ibd.meta

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

All comments were taken into attention.

[Laurynas Biveinis (laurynas-biveinis)]

The "inserting more data" step pointed out by Alexey is still there.

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

> The "inserting more data" step pointed out by Alexey is still there.
Ah, You're right!

Alexey, what do you mean by telling that inserting data does not make changes to data and does not affect delta? I can see opposite situation.

[Alexey Kopytov (akopytov)]

On 03.09.12 15:51, Sergei Glushchenko wrote:
>> The "inserting more data" step pointed out by Alexey is still there.
> Ah, You're right!
>
> Alexey, what do you mean by telling that inserting data does not make changes to data and does not affect delta? I can see opposite situation.
>

I mean that those 3 rows inserted after the full backup will be in redo
log, but not on disk and thus, not in delta. See bug #1021249.

The purpose of this test case is to make sure incremental apply doesn't
fail when space_id in .meta is missing. So it doesn't really matter if
the "inserting more data" step has any effect. That is, it can be
removed as it doesn't add anything to the test case objectives.

[Sergei Glushchenko (sergei.glushchenko)]

> I mean that those 3 rows inserted after the full backup will be in redo
> log, but not on disk and thus, not in delta. See bug #1021249.
>
I see. It depends on environment.

> The purpose of this test case is to make sure incremental apply doesn't
> fail when space_id in .meta is missing. So it doesn't really matter if
> the "inserting more data" step has any effect. That is, it can be
> removed as it doesn't add anything to the test case objectives.

The very first version of testcase also verified checksums in order to make sure that
matching by name worked correctly in this case. But now it really doesn't make sense.

[Sergei Glushchenko (sergei.glushchenko)]

Stage "insering more data" been completely removed.

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

[Alexey Kopytov (akopytov)]

review: Approve

=== modified file 'src/xtrabackup.c'
--- src/xtrabackup.c	2012-08-02 04:21:33 +0000
+++ src/xtrabackup.c	2012-09-05 08:34:20 +0000
@@ -2848,10 +2848,14 @@
 static my_bool
 xb_read_delta_metadata(const char *filepath, xb_delta_info_t *info)
 {
-	FILE *fp;
-	my_bool r= TRUE;
+	FILE*	fp;
+	char	key[51];
+	char	value[51];
+	my_bool	r			= TRUE;
 
-	memset(info, 0, sizeof(xb_delta_info_t));
+	/* set defaults */
+	info->page_size = ULINT_UNDEFINED;
+	info->space_id = ULINT_UNDEFINED;
 
 	fp = fopen(filepath, "r");
 	if (!fp) {
@@ -2859,12 +2863,28 @@
 		return(TRUE);
 	}
 
-	if (fscanf(fp, "page_size = %lu\nspace_id = %lu\n",
-		&info->page_size, &info->space_id) != 2)
-		r= FALSE;
+	while (!feof(fp)) {
+		if (fscanf(fp, "%50s = %50s\n", key, value) == 2) {
+			if (strcmp(key, "page_size") == 0) {
+				info->page_size = strtoul(value, NULL, 10);
+			} else if (strcmp(key, "space_id") == 0) {
+				info->space_id = strtoul(value, NULL, 10);
+			}
+		}
+	}
 
 	fclose(fp);
 
+	if (info->page_size == ULINT_UNDEFINED) {
+		msg("xtrabackup: page_size is required in %s\n", filepath);
+		r = FALSE;
+	}
+	if (info->space_id == ULINT_UNDEFINED) {
+		msg("xtrabackup: Warning: This backup was taken with XtraBackup 2.0.1 "
+			"or earlier, some DDL operations between full and incremental "
+			"backups may be handled incorrectly\n");
+	}
+
 	return(r);
 }
 
@@ -5657,7 +5677,7 @@
 	os_file_t	file	= 0;
 	ulint		tablespace_flags;
 
-	ut_a(dbname || space_id == 0);
+	ut_a(dbname != NULL || space_id == 0 || space_id == ULINT_UNDEFINED);
 
 	*success = FALSE;
 
@@ -5691,7 +5711,7 @@
 	mutex_exit(&fil_system->mutex);
 
 	if (fil_space != NULL) {
-		if (fil_space->id == space_id) {
+		if (fil_space->id == space_id || space_id == ULINT_UNDEFINED) {
 			/* we found matching space */
 			goto found;
 		} else {
@@ -5715,6 +5735,7 @@
 	}
 
 	mutex_enter(&fil_system->mutex);
+	ut_a(space_id != ULINT_UNDEFINED);
 	fil_space = xb_space_get_by_id(space_id);
 	mutex_exit(&fil_system->mutex);
 	if (fil_space != NULL) {

=== added file 'test/t/bug1038127.sh'
--- test/t/bug1038127.sh	1970-01-01 00:00:00 +0000
+++ test/t/bug1038127.sh	2012-09-05 08:34:20 +0000
@@ -0,0 +1,41 @@
+############################################################################
+# Bug #1038127: XtraBackup 2.0.2 is not backwards compatible
+#               if no space_id found in .meta file, applying delta
+#               to full backup failing
+############################################################################
+
+. inc/common.sh
+
+start_server --innodb_file_per_table
+
+run_cmd $MYSQL $MYSQL_ARGS test <<EOF
+CREATE TABLE t1(a INT) ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1), (2), (3);
+EOF
+
+# Full backup
+# backup root directory
+vlog "Starting backup"
+innobackupex  --no-timestamp $topdir/full
+
+vlog "Creating incremental backup"
+
+innobackupex --incremental --no-timestamp \
+    --incremental-basedir=$topdir/full $topdir/inc
+
+# remove space_id = something line from .meta file
+sed -ie '/space_id/ d' $topdir/inc/test/t1.ibd.meta
+
+vlog "Preparing backup"
+
+innobackupex --apply-log --redo-only $topdir/full
+vlog "Log applied to full backup"
+
+innobackupex --apply-log --redo-only --incremental-dir=$topdir/inc \
+    $topdir/full
+vlog "Delta applied to full backup"
+
+innobackupex --apply-log $topdir/full
+vlog "Data prepared for restore"
+
+grep -q "This backup was taken with XtraBackup 2.0.1" $OUTFILE