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

[Alexey Kopytov]

    Bug #950334: disk space grows by >500% while preparing a backup

    The fix for bug #461099 introduced the following logic in xtrabackup
    --prepare: after applying the log, we walk through tablespaces and
    adjust their sizes based on the FSP_SIZE value stored in the FSP
    header. This was required to prevent cases when a tablespace is extended
    while it is being copied by xtrabackup. In such cases InnoDB updates
    FSP_SIZE and extends the tablespace to the required size. However,
    unlike FSP_SIZE modifications, tablespace extensions are not
    redo-logged. Which means we get inconsistency between FSP_SIZE and the
    actual tablespace size after applying the log and that's why adjustments
    introduced by the fix for bug #461099 are necessary.

    The problem with that code was that it reads the first page of each
    tablespace unconditionally, i.e. even for tablespace that have not been
    touched (and thus loaded) by recovery. The code is also mostly a
    copy-paste from a UNIV_HOTBACKUP-only function
    fil_extend_tablespaces_to_stored_len(). And XtraBackup patches add the
    following logic to _fil_io(): if a request to read a page is beyond the
    tablespace size (which is 0 in the case being considered), extend the
    tablespace by aligning it to the nearest 1M boundary.

    One way to fix it would be to throw away that extra _fil_io() logic from
    XtraBackup patches and reuse fil_extend_tablespaces_to_stored_len() as
    the fix for bug #461099. That, however, would require a rather massive
    patch update to revert _fil_io() changes and make the
    UNIV_HOTBACKUP-only function (and any auxiliary functions it calls)
    available.

    Another approach is to only look for tablespaces touched by recovery
    when extending tablespaces, i.e. extend only those for which space->size
    > 0. This doesn't involve updating patches, and has an extra benefit
    that we will do less work by not processing unmodified spaces. That,
    however, doesn't work reliably. In some cases recovery wants to read in
    pages before it opens the tablespace, and that's where file extension
    logic in _fil_io() is triggered again and the file is unnecessarily
    extended.

    The third approach is to fix fil_io() so that tablespaces that have not
    yet been opened (i.e. with space->size = 0) are not extended. This,
    however, is not always sufficient. Even if we know the tablespace size
    already, and recovery wants to access a page beyond the current size, we
    would still align it to the file extent boundary (1M). But InnoDB itself
    doesn't do that for small tablespaces (i.e. less than 1M). Aligning file
    extensions to 1M was implemented in rev. 29 to make sure larger
    tablespaces are aligned as InnoDB expects them. However, that change was
    essentially obsoleted by the fix for bug #461099 (rev. 35), because with
    that change tablespaces are always aligned to what is actually stored in
    the FSP header and thus, matches what InnoDB expects them to be. So in
    addition to checking space->size the 1M alignment rule from rev. 29
    should be reverted.

    This revision implements the latter approach.

    It also looks like under some circumstances xtrabackup can extend the
    source file during backup with that extra code in fil_io() added by the
    XB patches. But this is a separate issue reported as #1144874 for
    further investigation. For now the test case in this patch has been
    adjusted so it doesn't hit that bug.

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

[Sergei Glushchenko (sergei.glushchenko)]

Approve. Thanks for very descriptive comments which made the review much more simple! Btw, innodb56.patch contains this fix already, which is not mentioned neither in this MP, nor in revision commit.

review: Approve (g2)

=== modified file 'patches/innodb51.patch'
--- patches/innodb51.patch	2012-11-14 04:46:11 +0000
+++ patches/innodb51.patch	2013-03-05 13:10:28 +0000
@@ -607,12 +607,12 @@
  
  	ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
  
-+	if (space->size <= block_offset) {
++	if (space->size > 0 && space->size <= block_offset) {
 +		ulint	actual_size;
 +
 +		mutex_exit(&fil_system->mutex);
 +		fil_extend_space_to_desired_size(&actual_size, space->id,
-+						 ((block_offset + 1) / 64 + 1) * 64);
++						 block_offset + 1);
 +		mutex_enter(&fil_system->mutex);
 +		/* should retry? but it may safe for xtrabackup for now. */
 +	}

=== modified file 'patches/innodb51_builtin.patch'
--- patches/innodb51_builtin.patch	2012-07-18 15:00:20 +0000
+++ patches/innodb51_builtin.patch	2013-03-05 13:10:28 +0000
@@ -577,12 +577,12 @@
  
  	ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
  
-+	if (space->size <= block_offset) {
++	if (space->size > 0 && space->size <= block_offset) {
 +		ulint	actual_size;
 +
 +		mutex_exit(&(system->mutex));
 +		fil_extend_space_to_desired_size(&actual_size, space->id,
-+						 ((block_offset + 1) / 64 + 1) * 64);
++						 block_offset + 1);
 +		mutex_enter(&(system->mutex));
 +		/* should retry? but it may safe for xtrabackup for now. */
 +	}

=== modified file 'patches/innodb55.patch'
--- patches/innodb55.patch	2012-02-22 16:37:18 +0000
+++ patches/innodb55.patch	2013-03-05 13:10:28 +0000
@@ -585,12 +585,12 @@
  
  	ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
  
-+	if (space->size <= block_offset) {
++	if (space->size > 0 && space->size <= block_offset) {
 +		ulint	actual_size;
 +
 +		mutex_exit(&fil_system->mutex);
 +		fil_extend_space_to_desired_size(&actual_size, space->id,
-+						 ((block_offset + 1) / 64 + 1) * 64);
++						 block_offset + 1);
 +		mutex_enter(&fil_system->mutex);
 +		/* should retry? but it may safe for xtrabackup for now. */
 +	}

=== modified file 'patches/xtradb51.patch'
--- patches/xtradb51.patch	2012-02-22 16:37:18 +0000
+++ patches/xtradb51.patch	2013-03-05 13:10:28 +0000
@@ -489,12 +489,12 @@
  
  	ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
  
-+	if (space->size <= block_offset) {
++	if (space->size > 0 && space->size <= block_offset) {
 +		ulint	actual_size;
 +
 +		mutex_exit(&fil_system->mutex);
 +		fil_extend_space_to_desired_size(&actual_size, space->id,
-+						 ((block_offset + 1) / 64 + 1) * 64);
++						 block_offset + 1);
 +		mutex_enter(&fil_system->mutex);
 +		/* should retry? but it may safe for xtrabackup for now. */
 +	}

=== modified file 'patches/xtradb55.patch'
--- patches/xtradb55.patch	2013-03-01 06:54:51 +0000
+++ patches/xtradb55.patch	2013-03-05 13:10:28 +0000
@@ -477,12 +477,12 @@
  
  	ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
  
-+	if (space->size <= block_offset) {
++	if (space->size > 0 && space->size <= block_offset) {
 +		ulint	actual_size;
 +
 +		mutex_exit(&fil_system->mutex);
 +		fil_extend_space_to_desired_size(&actual_size, space->id,
-+						 ((block_offset + 1) / 64 + 1) * 64);
++						 block_offset + 1);
 +		mutex_enter(&fil_system->mutex);
 +		/* should retry? but it may safe for xtrabackup for now. */
 +	}

=== added file 'test/t/bug950334.sh'
--- test/t/bug950334.sh	1970-01-01 00:00:00 +0000
+++ test/t/bug950334.sh	2013-03-05 13:10:28 +0000
@@ -0,0 +1,26 @@
+########################################################################
+# Bug #950334: disk space grows by >500% while preparing a backup
+########################################################################
+
+. inc/common.sh
+
+start_server --innodb_file_per_table
+
+run_cmd $MYSQL $MYSQL_ARGS test <<EOF
+CREATE TABLE t (a INT) ENGINE=InnoDB;
+INSERT INTO t VALUES (1), (2), (3);
+EOF
+
+innobackupex --no-timestamp $topdir/backup
+
+innobackupex --apply-log $topdir/backup
+
+# The size of t.ibd should be 96K, not 1M
+size_src=`ls -l $MYSQLD_DATADIR/test/t.ibd | awk '{print $5}'`
+size_dst=`ls -l $topdir/backup/test/t.ibd | awk '{print $5}'`
+
+if [ "$size_src" != "$size_dst" ]; then
+    vlog "t.ibd has different sizes in source ($size_src bytes) and \
+backup ($size_dst) directories"
+    exit -1
+fi