Merge lp:~laurynas-biveinis/percona-xtrabackup/bug1044398-2.0 into lp:percona-xtrabackup/2.0

[Laurynas Biveinis]

Fix bug 1044398 (Handling of compressed tablespaces with compressed
page size == server page size broken)

The problem was that incremental backup table metadata format,
after fixing bug 932623, did not contain a compression flag nor a
compressed page size. Instead, it stored only a page size, and
compression was implied whenever page size < UNIV_PAGE_SIZE. This
assumption breaks with compressed page size == UNIV_PAGE_SIZE.

Fixed by adding storing a new field zip_size in the metadata. Its
presence is optional for backwards compatibility: when it's absent,
XtraBackup reverts to the old logic.

Added new test bug1044398 for testing this backwards compatibility.

Jenkins: http://jenkins.percona.com/job/percona-xtrabackup-2.0-param/253/

[Alexey Kopytov (akopytov)]

Why does the test case creates an uncompressed table with the 16K page size? As I understand, a compressed 16K page size was the broken case?

review: Needs Information

[Laurynas Biveinis (laurynas-biveinis)]

Alexey - yes, but this test tests compatibility with metadata that lacks zip_size, and 16K case is tested to see that it is correctly treated as uncompressed.

[Alexey Kopytov (akopytov)]

Shouldn't we then have a test case that tests the case from the bug report, i.e. a compressed 16K page.

[Alexey Kopytov (akopytov)]

I also don't think we should test _all_ possible page sizes just for compatibility with 2.0.2. Just one 16K page size with no zip_size in metadata should be enough.

review: Needs Fixing

[Laurynas Biveinis (laurynas-biveinis)]

Alexey - xb_incremental_compressed tests this case. At this code revision this bug simply does not cause anything wrong, since tablespace creation is dumb byte copy anyway. However, in my follow-up MP that introduces fil_create_new_single_file_tablespace()-like code, xb_incremental_compressed starts failing if the current fix is not applied.

[Laurynas Biveinis (laurynas-biveinis)]

I believe we must test at least one < 16K case for backwards compatibility. Let's say 1K and 16K then?

[Alexey Kopytov (akopytov)]

xb_incremental_compressed might be changed at some point, so having an explicit test case covering the specific case from a bug is always a good thing. Naturally, there's a lot of overlapping tests in the MySQL test suite testing the same code paths.

[Alexey Kopytov (akopytov)]

On 07.09.12 17:40, Laurynas Biveinis wrote:
> I believe we must test at least one < 16K case for backwards compatibility. Let's say 1K and 16K then?
>

Was there ever a problem with 1K pages?

[Laurynas Biveinis (laurynas-biveinis)]

OK, I will make a feature test for 16K compressed page. Then we'd have two tests: this feature test and the backwards compatibility test (might live in the same .sh file of course).

> > I believe we must test at least one < 16K case for backwards compatibility.
> Let's say 1K and 16K then?
> >
>
> Was there ever a problem with 1K pages?

No. It's defensive: if we introduced a regression where we'd start treating small page_size zip_size-less metadatas as uncompressed ones, we'd never know without at least one such test. As this is purely backward file format compatibility test, I don't see a difference between this case and 16K page case.

[lp:~laurynas-biveinis/percona-xtrabackup/bug1044398-2.0]

466. By Stewart Smith on 2012-09-10

merge fix for Bug #1038127: XtraBackup 2.0.2 is not backwards compatible

467. By Laurynas Biveinis on 2012-09-11

Fix bug 1044398 (Handling of compressed tablespaces with compressed
page size == server page size broken)

The problem was that incremental backup table metadata format,
after fixing bug 932623, did not contain a compression flag nor a
compressed page size. Instead, it stored only a page size, and
compression was implied whenever page size < UNIV_PAGE_SIZE. This
assumption breaks with compressed page size == UNIV_PAGE_SIZE.

Fixed by storing a new field zip_size in the metadata and using it for
tablespace creation instead of the assumptions.

Added new test bug1044398 to assert that zip_size absence in the delta
metadata is fatal.

Unmerged revisions

467. By Laurynas Biveinis on 2012-09-11

Fix bug 1044398 (Handling of compressed tablespaces with compressed
page size == server page size broken)

The problem was that incremental backup table metadata format,
after fixing bug 932623, did not contain a compression flag nor a
compressed page size. Instead, it stored only a page size, and
compression was implied whenever page size < UNIV_PAGE_SIZE. This
assumption breaks with compressed page size == UNIV_PAGE_SIZE.

Fixed by storing a new field zip_size in the metadata and using it for
tablespace creation instead of the assumptions.

Added new test bug1044398 to assert that zip_size absence in the delta
metadata is fatal.

=== modified file 'src/xtrabackup.c'
--- src/xtrabackup.c	2012-09-05 08:32:02 +0000
+++ src/xtrabackup.c	2012-09-11 10:20:52 +0000
@@ -655,6 +655,7 @@
 
 typedef struct {
 	ulint	page_size;
+	ulint	zip_size;
 	ulint	space_id;
 } xb_delta_info_t;
 
@@ -2855,6 +2856,7 @@
 
 	/* set defaults */
 	info->page_size = ULINT_UNDEFINED;
+	info->zip_size = ULINT_UNDEFINED;
 	info->space_id = ULINT_UNDEFINED;
 
 	fp = fopen(filepath, "r");
@@ -2867,6 +2869,8 @@
 		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, "zip_size") == 0) {
+				info->zip_size = strtoul(value, NULL, 10);
 			} else if (strcmp(key, "space_id") == 0) {
 				info->space_id = strtoul(value, NULL, 10);
 			}
@@ -2879,6 +2883,10 @@
 		msg("xtrabackup: page_size is required in %s\n", filepath);
 		r = FALSE;
 	}
+	if (info->zip_size == ULINT_UNDEFINED) {
+		msg("xtrabackup: zip_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 "
@@ -2904,8 +2912,10 @@
 	MY_STAT		mystat;
 
 	snprintf(buf, sizeof(buf),
-		 "page_size = %lu\nspace_id = %lu\n",
-		 info->page_size, info->space_id);
+		 "page_size = %lu\n"
+		 "zip_size = %lu\n"
+		 "space_id = %lu\n",
+		 info->page_size, info->zip_size, info->space_id);
 	len = strlen(buf);
 
 	mystat.st_size = len;
@@ -3103,14 +3113,12 @@
 	byte*		incremental_buffer_base = NULL;
 	ulint		page_size;
 	ulint		page_size_shift;
-#ifdef INNODB_VERSION_SHORT
-	ulint		zip_size;
-#endif
 	xb_delta_info_t info;
 	datasink_t	*ds = ds_ctxt->datasink;
 	ds_file_t	*dstfile = NULL;
 
 	info.page_size = 0;
+	info.zip_size = 0;
 	info.space_id = 0;
 
 #ifdef XTRADB_BASED
@@ -3257,11 +3265,11 @@
 	page_size = UNIV_PAGE_SIZE;
 	page_size_shift = UNIV_PAGE_SIZE_SHIFT;
 #else
-	zip_size = xb_get_zip_size(src_file);
-	if (zip_size == ULINT_UNDEFINED) {
+	info.zip_size = xb_get_zip_size(src_file);
+	if (info.zip_size == ULINT_UNDEFINED) {
 		goto skip;
-	} else if (zip_size) {
-		page_size = zip_size;
+	} else if (info.zip_size) {
+		page_size = info.zip_size;
 		page_size_shift = get_bit_shift(page_size);
 		msg("[%02u] %s is compressed with page size = "
 		    "%lu bytes\n", thread_n, node->name, page_size);
@@ -3367,7 +3375,8 @@
 #ifndef INNODB_VERSION_SHORT
 			if (buf_page_is_corrupted(page + chunk_offset))
 #else
-			if (buf_page_is_corrupted(page + chunk_offset, zip_size))
+			if (buf_page_is_corrupted(page + chunk_offset,
+						  info.zip_size))
 #endif
 			{
 				if (
@@ -5911,8 +5920,7 @@
 	xb_file_set_nocache(src_file, src_path, "OPEN");
 
 	dst_file = xb_delta_open_matching_space(
-			dbname, space_name, info.space_id,
-			info.page_size == UNIV_PAGE_SIZE ? 0 : info.page_size,
+			dbname, space_name, info.space_id, info.zip_size,
 			dst_path, sizeof(dst_path), &success);
 	if (!success) {
 		msg("xtrabackup: error: cannot open %s\n", dst_path);
 
=== added file 'test/t/bug1044398.sh'
--- test/t/bug1044398.sh	1970-01-01 00:00:00 +0000
+++ test/t/bug1044398.sh	2012-09-11 10:20:52 +0000
@@ -0,0 +1,37 @@
+# Test compatibility for pre-bug 1044398 incremental meta files.
+
+. inc/common.sh
+
+if [ -z "$INNODB_VERSION" ]; then
+    echo "Requires InnoDB plugin or XtraDB" >$SKIPPED_REASON
+    exit $SKIPPED_EXIT_CODE
+fi
+
+start_server --innodb_file_per_table
+
+vlog "Creating full backup"
+innobackupex --no-timestamp $topdir/full
+
+vlog "Creating new tablespace"
+run_cmd ${MYSQL} ${MYSQL_ARGS} -e \
+    "CREATE TABLE t1(a INT) ENGINE=InnoDB" test
+    
+run_cmd ${MYSQL} ${MYSQL_ARGS} -e \
+    "INSERT INTO t1 VALUES (1)" test
+
+vlog "Creating incremental backup"
+innobackupex --incremental --no-timestamp \
+    --incremental-basedir=$topdir/full $topdir/inc
+
+# Remove zip_size = $page_size line from .meta file
+sed -ie '/zip_size/ d' $topdir/inc/test/t1.ibd.meta
+
+vlog "Preparing backup, applying log"
+innobackupex --apply-log -redo-only $topdir/full
+
+vlog "Preparing backup, applying delta, should fail with missing zip_size error"
+run_cmd_expect_failure $IB_BIN $IB_ARGS --apply-log --redo-only --incremental-dir=$topdir/inc $topdir/full
+
+grep -q "xtrabackup: zip_size is required " $OUTFILE
+
+stop_server