Merge lp:~sergei.glushchenko/percona-xtrabackup/BT-26901-2.0 into lp:percona-xtrabackup/2.0

[Stewart Smith]

Bug #1066843: Fix for bug #932623 does not take separate doublewrite
tablespace into account

Fixed by checking trx_sys_sys_space instead of space_id == 0.
innodb_doublewrite_file been added to backup-my.cnf.
Copy back now skip doublewrite tablespace.

lp:~sergei.glushchenko/percona-xtrabackup/BT-26901-2.0

[Sergei Glushchenko (sergei.glushchenko)]

#26901

[Laurynas Biveinis (laurynas-biveinis)]

What is the purpose of chunk at 23-40?

That chunk and the innobackupex change seem to fix some other bug, not a regression from 932623. Can you link that bug, or if there is none, report it?

Please introduce a wrapper around #ifdef ... == 0 #else ... trx_sys_sys_space ... #endif.

Why does the testcase skip in case of !XtraDB? I think this test applies to all InnoDB flavors.

Is it necessary to grep innobackupex output for full_backup_dir? IIRC other testcases dont't do this. Likewise for inc_backup_dir.

I am not sure if the testcase should be all the way to table checksum checking with restored data, or the steps up to the crashing point are necessary. In this particular instance I'd err on the side of more testing.

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

> What is the purpose of chunk at 23-40?
>
> That chunk and the innobackupex change seem to fix some other bug, not a
> regression from 932623. Can you link that bug, or if there is none, report it?
>

OK. I'll report separate bug for that. Or maybe it's possible to change description of existing one?

> Please introduce a wrapper around #ifdef ... == 0 #else ... trx_sys_sys_space
> ... #endif.
>

This can be done.

> Why does the testcase skip in case of !XtraDB? I think this test applies to
> all InnoDB flavors.
>

It's because of usage of innodb_doublewrite_file.

> Is it necessary to grep innobackupex output for full_backup_dir? IIRC other
> testcases dont't do this. Likewise for inc_backup_dir.
>

Yep. Could be changed to use --no-timestamps

> I am not sure if the testcase should be all the way to table checksum checking
> with restored data, or the steps up to the crashing point are necessary. In
> this particular instance I'd err on the side of more testing.

Basically I had failures at last server startup. This is what 23-40 and innobackupex changes are needed.

[Sergei Glushchenko (sergei.glushchenko)]

Explanation of changes in innobackupex is following.

During backup xtrabackup copies doublewrite buffer file from the path specified in my.cnf to the root backup directory. Then during prepare double file is not specified in backup-my.cnf, and not used. When copy back is done old doublewrite buffer file copied to root data directory, which could cause failures on server startup.

Following are incorrect and being fixed:
1. Path to doublewrite buffer file is not specified in backup-my.cnf
2. Prepare should use doublewrite buffer file from root of backup dir
3. Copy back should not copy doublewrite buffer file as it is not needed after prepare is done.

[Sergei Glushchenko (sergei.glushchenko)]

In the case of run with our test framework we have even worst behavior. Instead of looking at backup-my.cnf prepare look at original my.cnf (--defaults-file is specified) and modify original doublewrite buffer file.

[Sergei Glushchenko (sergei.glushchenko)]

Bug failed and linked, testcase changed to use --no-timestamp, wrapper for trx_sys_sys_space introduced.

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

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

[Alexey Kopytov (akopytov)]

Sergei,

A few of comments:

- what if innodb_doublewrite_file is not specified in my.cnf? As I understand, --print-param will either crash or print "innodb_doublewrite_file = (null)" (on Linux), and then innobackupex will assume we have a doublewrite full with "(null)" as its name.

- why don't we write stripped innodb_doublewrite_file (i.e. with path removed) to backup-my.cnf? Less code in innobackupex to strip it on --copy-back

- why don't we make 2.0 and 2.1 consistent and just modify trx_sys_sys_space(id) to include the #ifdef and avoid having a separate macro? Less code changes in 2.0, less conflicts when merging 2.0->2.1 in the future.

- please use one of MYSQLD_VARDIR/MYSQLD_DATADIR/MYSQLD_TMPDIR instead of hardcoded paths as in "${TEST_BASEDIR}/var1..."

- s/.idb/.ibd/

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

> Sergei,
>
> A few of comments:
>
> - what if innodb_doublewrite_file is not specified in my.cnf? As I understand,
> --print-param will either crash or print "innodb_doublewrite_file = (null)"
> (on Linux), and then innobackupex will assume we have a doublewrite full with
> "(null)" as its name.
>

Yes, i fixed it.

> - why don't we write stripped innodb_doublewrite_file (i.e. with path removed)
> to backup-my.cnf? Less code in innobackupex to strip it on --copy-back
>

The reason was to avoid hacking in testcase. Also we modified some values from my.cnf when put them to backup-my.cnf before. Currently we don't. But I agree that full original path is not needed in backup-my.cnf for xtrabackup to operate. So I rewrite to do path strip before writing to backup-my.cnf.

> - why don't we make 2.0 and 2.1 consistent and just modify
> trx_sys_sys_space(id) to include the #ifdef and avoid having a separate macro?
> Less code changes in 2.0, less conflicts when merging 2.0->2.1 in the future.
>

Yep. The reason was I did not see it before. Then see in 2.1 and fix and not yet synced 2.0 with it.

> - please use one of MYSQLD_VARDIR/MYSQLD_DATADIR/MYSQLD_TMPDIR instead of
> hardcoded paths as in "${TEST_BASEDIR}/var1..."
>

It isn't possible due to unavailability of MYSQLD_* before server startup.

> - s/.idb/.ibd/

OK.

[Alexey Kopytov (akopytov)]

Sergei,

Thanks, everything looks good now, except that I think we can still avoid hard-coded path for the doublewrite file.

On server startup, the path can be relative to the data, i.e. --innodb-doublewrite-path=foo.ibd. If you need a full path to it later (I don't think you actually need, but still), you can refer to it as $MYSQL_DATADIR/foo.ibd.

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

Testcase been modified. Path to dblwr.ibd is not specified now.

=== modified file 'innobackupex'
--- innobackupex	2012-11-07 06:44:45 +0000
+++ innobackupex	2012-11-15 15:30:29 +0000
@@ -719,6 +719,13 @@
     my $file;
     my $backup_innodb_data_file_path;
 
+    if (has_option(\%config, $option_defaults_group, 'innodb_doublewrite_file')) {
+        my $doublewrite_file =
+                get_option(\%config, $option_defaults_group,
+                            'innodb_doublewrite_file');
+        $excluded_files = $excluded_files . '|' . $doublewrite_file;
+    }
+
     # check whether files should be copied or moved to dest directory
     my $move_or_copy_file = $move_flag ? \&move_file : \&copy_file;
     my $move_or_copy_dir = $move_flag ?
@@ -1727,7 +1734,7 @@
         "innodb_log_file_size",
         "innodb_fast_checksum",
         "innodb_page_size",
-        "innodb_log_block_size",
+        "innodb_log_block_size"
         );
 
     my $options_dump = "# This MySQL options file was generated by $innobackup_script.\n\n" .
@@ -1741,6 +1748,12 @@
             $options_dump .= "$option_name=$option_value\n";
         }
     }
+    if (has_option(\%config,
+        $option_defaults_group, "innodb_doublewrite_file")) {
+        $options_dump .= "innodb_doublewrite_file=" . (split(/\/+/,
+                get_option(\%config, $option_defaults_group,
+                            'innodb_doublewrite_file')))[-1] . "\n";
+    }
 
     print FILE $options_dump;
     close(FILE);

=== modified file 'src/xtrabackup.c'
--- src/xtrabackup.c	2012-11-08 23:42:03 +0000
+++ src/xtrabackup.c	2012-11-15 15:30:29 +0000
@@ -2970,6 +2970,10 @@
 	HASH_SEARCH(NAME, TABLE, FOLD, DATA, TEST)
 #endif
 
+#ifndef XTRADB_BASED
+#define trx_sys_sys_space(id) (id == 0)
+#endif
+
 /****************************************************************//**
 A simple function to open or create a file.
 @return own: handle to the file, not defined if error, error number
@@ -3137,11 +3141,7 @@
 	info.zip_size = 0;
 	info.space_id = 0;
 
-#ifdef XTRADB_BASED
 	if (xtrabackup_tables && (!trx_sys_sys_space(node->space->id)))
-#else
-	if (xtrabackup_tables && (node->space->id != 0))
-#endif
 	{ /* must backup id==0 */
 		char *p;
 		int p_len, regres = REG_NOMATCH;
@@ -3185,11 +3185,7 @@
 		}
 	}
 
-#ifdef XTRADB_BASED
 	if (xtrabackup_tables_file && (!trx_sys_sys_space(node->space->id)))
-#else
-	if (xtrabackup_tables_file && (node->space->id != 0))
-#endif
 	{ /* must backup id==0 */
 		xtrabackup_tables_t* table;
 		char *p;
@@ -3231,11 +3227,7 @@
 
 skip_filter:
 
-#ifdef XTRADB_BASED
 	if (trx_sys_sys_space(node->space->id))
-#else
-	if (node->space->id == 0)
-#endif
 	{
 		char *next, *p;
 		/* system datafile "/fullpath/datafilename.ibd" or "./datafilename.ibd" */
@@ -3396,11 +3388,7 @@
 #endif
 			{
 				if (
-#ifdef XTRADB_BASED
 				    trx_sys_sys_space(node->space->id)
-#else
-				    node->space->id == 0
-#endif
 				    && ((offset + (IB_INT64)chunk_offset) >> page_size_shift)
 				       >= FSP_EXTENT_SIZE
 				    && ((offset + (IB_INT64)chunk_offset) >> page_size_shift)
@@ -4060,11 +4048,7 @@
 	} else {
 		ptr2 = NULL;
 	}
-#ifdef XTRADB_BASED
 	if(!trx_sys_sys_space(space->id) && ptr2)
-#else
-	if(space->id && ptr2)
-#endif
 	{
 		/* single table space */
 		*ptr2 = 0; /* temporary (it's my lazy..)*/
@@ -5794,7 +5778,9 @@
 	os_file_t	file	= 0;
 	ulint		tablespace_flags;
 
-	ut_a(dbname != NULL || space_id == 0 || space_id == ULINT_UNDEFINED);
+	ut_a(dbname != NULL ||
+		trx_sys_sys_space(space_id) ||
+		space_id == ULINT_UNDEFINED);
 
 	*success = FALSE;
 
@@ -5823,6 +5809,10 @@
 		return file;
 	}
 
+	if (trx_sys_sys_space(space_id)) {
+		goto found;
+	}
+
 	mutex_enter(&fil_system->mutex);
 	fil_space = xb_space_get_by_name(dest_space_name);
 	mutex_exit(&fil_system->mutex);
@@ -6552,11 +6542,7 @@
 			while (space != NULL) {
 				/* treat file_per_table only */
 				if (space->purpose != FIL_TABLESPACE
-#ifdef XTRADB_BASED
 				    || trx_sys_sys_space(space->id)
-#else
-				    || space->id == 0
-#endif
 				   )
 				{
 					space = UT_LIST_GET_NEXT(space_list, space);
@@ -7089,6 +7075,9 @@
 		printf("innodb_fast_checksum = %d\n", innobase_fast_checksum);
 		printf("innodb_page_size = %ld\n", innobase_page_size);
 		printf("innodb_log_block_size = %lu\n", innobase_log_block_size);
+		if (innobase_doublewrite_file != NULL) {
+			printf("innodb_doublewrite_file = %s\n", innobase_doublewrite_file);
+		}
 #endif
 		exit(EXIT_SUCCESS);
 	}

=== added file 'test/t/ib_doublewrite.sh'
--- test/t/ib_doublewrite.sh	1970-01-01 00:00:00 +0000
+++ test/t/ib_doublewrite.sh	2012-11-15 15:30:29 +0000
@@ -0,0 +1,120 @@
+########################################################################
+# Bug #1066843: Fix for bug #932623 does not take separate doublewrite
+#               tablespace into account
+# Bug #1068470: XtraBackup handles separate doublewrite buffer file
+#               incorrectly
+# We testing full and incremental backup and restore to check that
+# separate doublewrite buffer file handled correctly
+########################################################################
+
+. inc/common.sh
+
+if [ -z "$XTRADB_VERSION" ]; then
+    echo "Requires XtraDB" > $SKIPPED_REASON
+    exit $SKIPPED_EXIT_CODE
+fi
+
+DBLWR=dblwr.ibd
+start_server --innodb_file_per_table --innodb_doublewrite_file=${DBLWR}
+load_dbase_schema incremental_sample
+
+# Workaround for bug #1072695
+IB_ARGS_NO_DEFAULTS_FILE=`echo $IB_ARGS | sed -e 's/--defaults-file=[^ ]* / /'`
+function innobackupex_no_defaults_file ()
+{
+	run_cmd $IB_BIN $IB_ARGS_NO_DEFAULTS_FILE $*
+}
+
+echo "innodb_doublewrite_file=${DBLWR}" >>$topdir/my.cnf
+
+# Adding initial rows
+vlog "Adding initial rows to database..."
+${MYSQL} ${MYSQL_ARGS} -e "insert into test values (1, 1);" incremental_sample
+
+# Full backup
+# backup root directory
+mkdir -p $topdir/backup
+
+vlog "Starting backup"
+full_backup_dir=$topdir/backup/full
+innobackupex  --no-timestamp $full_backup_dir
+vlog "Full backup done to directory $full_backup_dir"
+cat $full_backup_dir/backup-my.cnf
+
+# Changing data
+
+vlog "Making changes to database"
+${MYSQL} ${MYSQL_ARGS} -e "create table t2 (a int(11) default null, number int(11) default null) engine=innodb" incremental_sample
+${MYSQL} ${MYSQL_ARGS} -e "insert into test values (10, 1);" incremental_sample
+${MYSQL} ${MYSQL_ARGS} -e "insert into t2 values (10, 1);" incremental_sample
+vlog "Changes done"
+
+# Saving the checksum of original table
+checksum_test_a=`checksum_table incremental_sample test`
+checksum_t2_a=`checksum_table incremental_sample t2`
+vlog "Table 'test' checksum is $checksum_test_a"
+vlog "Table 't2' checksum is $checksum_t2_a"
+
+vlog "Making incremental backup"
+
+vlog "###############"
+vlog "# INCREMENTAL #"
+vlog "###############"
+
+# Incremental backup
+inc_backup_dir=$topdir/backup/inc
+innobackupex --no-timestamp --incremental --incremental-basedir=$full_backup_dir \
+    $inc_backup_dir
+vlog "Incremental backup done to directory $inc_backup_dir"
+
+vlog "Preparing backup"
+# Prepare backup
+vlog "##############"
+vlog "# PREPARE #1 #"
+vlog "##############"
+innobackupex_no_defaults_file --apply-log --redo-only $full_backup_dir
+vlog "Log applied to full backup"
+vlog "##############"
+vlog "# PREPARE #2 #"
+vlog "##############"
+innobackupex_no_defaults_file --apply-log --redo-only --incremental-dir=$inc_backup_dir \
+    $full_backup_dir
+vlog "Delta applied to full backup"
+vlog "##############"
+vlog "# PREPARE #3 #"
+vlog "##############"
+innobackupex_no_defaults_file --apply-log $full_backup_dir
+vlog "Data prepared for restore"
+
+# Destroying mysql data
+stop_server
+rm -rf $mysql_datadir/*
+vlog "Data destroyed"
+
+# Restore backup
+vlog "Copying files"
+vlog "###########"
+vlog "# RESTORE #"
+vlog "###########"
+innobackupex --copy-back $full_backup_dir
+vlog "Data restored"
+
+start_server --innodb_file_per_table --innodb_doublewrite_file=${DBLWR}
+
+vlog "Checking checksums"
+checksum_test_b=`checksum_table incremental_sample test`
+checksum_t2_b=`checksum_table incremental_sample t2`
+
+if [ "$checksum_test_a" != "$checksum_test_b"  ]
+then 
+	vlog "Checksums for table 'test' are not equal"
+	exit -1
+fi
+
+if [ "$checksum_t2_a" != "$checksum_t2_b"  ]
+then 
+	vlog "Checksums for table 't2' are not equal"
+	exit -1
+fi
+
+vlog "Checksums are OK"