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

Proposed by Alexey Kopytov
Status: Merged
Approved by: Sergei Glushchenko
Approved revision: 503
Merged at revision: 516
Proposed branch: lp:~akopytov/percona-xtrabackup/bug950334-2.0
Merge into: lp:percona-xtrabackup/2.0
Diff against target: 125 lines (+36/-10)
6 files modified
patches/innodb51.patch (+2/-2)
patches/innodb51_builtin.patch (+2/-2)
patches/innodb55.patch (+2/-2)
patches/xtradb51.patch (+2/-2)
patches/xtradb55.patch (+2/-2)
test/t/bug950334.sh (+26/-0)
To merge this branch: bzr merge lp:~akopytov/percona-xtrabackup/bug950334-2.0
Reviewer Review Type Date Requested Status
Sergei Glushchenko (community) g2 Approve
Review via email: mp+151744@code.launchpad.net

Description of the change

    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/

To post a comment you must log in.
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'patches/innodb51.patch'
2--- patches/innodb51.patch 2012-11-14 04:46:11 +0000
3+++ patches/innodb51.patch 2013-03-05 13:10:28 +0000
4@@ -607,12 +607,12 @@
5
6 ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
7
8-+ if (space->size <= block_offset) {
9++ if (space->size > 0 && space->size <= block_offset) {
10 + ulint actual_size;
11 +
12 + mutex_exit(&fil_system->mutex);
13 + fil_extend_space_to_desired_size(&actual_size, space->id,
14-+ ((block_offset + 1) / 64 + 1) * 64);
15++ block_offset + 1);
16 + mutex_enter(&fil_system->mutex);
17 + /* should retry? but it may safe for xtrabackup for now. */
18 + }
19
20=== modified file 'patches/innodb51_builtin.patch'
21--- patches/innodb51_builtin.patch 2012-07-18 15:00:20 +0000
22+++ patches/innodb51_builtin.patch 2013-03-05 13:10:28 +0000
23@@ -577,12 +577,12 @@
24
25 ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
26
27-+ if (space->size <= block_offset) {
28++ if (space->size > 0 && space->size <= block_offset) {
29 + ulint actual_size;
30 +
31 + mutex_exit(&(system->mutex));
32 + fil_extend_space_to_desired_size(&actual_size, space->id,
33-+ ((block_offset + 1) / 64 + 1) * 64);
34++ block_offset + 1);
35 + mutex_enter(&(system->mutex));
36 + /* should retry? but it may safe for xtrabackup for now. */
37 + }
38
39=== modified file 'patches/innodb55.patch'
40--- patches/innodb55.patch 2012-02-22 16:37:18 +0000
41+++ patches/innodb55.patch 2013-03-05 13:10:28 +0000
42@@ -585,12 +585,12 @@
43
44 ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
45
46-+ if (space->size <= block_offset) {
47++ if (space->size > 0 && space->size <= block_offset) {
48 + ulint actual_size;
49 +
50 + mutex_exit(&fil_system->mutex);
51 + fil_extend_space_to_desired_size(&actual_size, space->id,
52-+ ((block_offset + 1) / 64 + 1) * 64);
53++ block_offset + 1);
54 + mutex_enter(&fil_system->mutex);
55 + /* should retry? but it may safe for xtrabackup for now. */
56 + }
57
58=== modified file 'patches/xtradb51.patch'
59--- patches/xtradb51.patch 2012-02-22 16:37:18 +0000
60+++ patches/xtradb51.patch 2013-03-05 13:10:28 +0000
61@@ -489,12 +489,12 @@
62
63 ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
64
65-+ if (space->size <= block_offset) {
66++ if (space->size > 0 && space->size <= block_offset) {
67 + ulint actual_size;
68 +
69 + mutex_exit(&fil_system->mutex);
70 + fil_extend_space_to_desired_size(&actual_size, space->id,
71-+ ((block_offset + 1) / 64 + 1) * 64);
72++ block_offset + 1);
73 + mutex_enter(&fil_system->mutex);
74 + /* should retry? but it may safe for xtrabackup for now. */
75 + }
76
77=== modified file 'patches/xtradb55.patch'
78--- patches/xtradb55.patch 2013-03-01 06:54:51 +0000
79+++ patches/xtradb55.patch 2013-03-05 13:10:28 +0000
80@@ -477,12 +477,12 @@
81
82 ut_ad((mode != OS_AIO_IBUF) || (space->purpose == FIL_TABLESPACE));
83
84-+ if (space->size <= block_offset) {
85++ if (space->size > 0 && space->size <= block_offset) {
86 + ulint actual_size;
87 +
88 + mutex_exit(&fil_system->mutex);
89 + fil_extend_space_to_desired_size(&actual_size, space->id,
90-+ ((block_offset + 1) / 64 + 1) * 64);
91++ block_offset + 1);
92 + mutex_enter(&fil_system->mutex);
93 + /* should retry? but it may safe for xtrabackup for now. */
94 + }
95
96=== added file 'test/t/bug950334.sh'
97--- test/t/bug950334.sh 1970-01-01 00:00:00 +0000
98+++ test/t/bug950334.sh 2013-03-05 13:10:28 +0000
99@@ -0,0 +1,26 @@
100+########################################################################
101+# Bug #950334: disk space grows by >500% while preparing a backup
102+########################################################################
103+
104+. inc/common.sh
105+
106+start_server --innodb_file_per_table
107+
108+run_cmd $MYSQL $MYSQL_ARGS test <<EOF
109+CREATE TABLE t (a INT) ENGINE=InnoDB;
110+INSERT INTO t VALUES (1), (2), (3);
111+EOF
112+
113+innobackupex --no-timestamp $topdir/backup
114+
115+innobackupex --apply-log $topdir/backup
116+
117+# The size of t.ibd should be 96K, not 1M
118+size_src=`ls -l $MYSQLD_DATADIR/test/t.ibd | awk '{print $5}'`
119+size_dst=`ls -l $topdir/backup/test/t.ibd | awk '{print $5}'`
120+
121+if [ "$size_src" != "$size_dst" ]; then
122+ vlog "t.ibd has different sizes in source ($size_src bytes) and \
123+backup ($size_dst) directories"
124+ exit -1
125+fi

Subscribers

People subscribed via source and target branches