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

Proposed by Sergei Glushchenko on 2012-11-15
Status: Merged
Approved by: Alexey Kopytov on 2012-11-15
Approved revision: 472
Merged at revision: 480
Proposed branch: lp:~sergei.glushchenko/percona-xtrabackup/BT-26901-2.0
Merge into: lp:percona-xtrabackup/2.0
Diff against target: 283 lines (+148/-26)
3 files modified
innobackupex (+14/-1)
src/xtrabackup.c (+14/-25)
test/t/ib_doublewrite.sh (+120/-0)
To merge this branch: bzr merge lp:~sergei.glushchenko/percona-xtrabackup/BT-26901-2.0
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) 2012-11-15 Approve on 2012-11-15
Laurynas Biveinis 2012-11-15 Pending
Review via email: mp+134512@code.launchpad.net

This proposal supersedes a proposal from 2012-11-12.

Description of the change

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

To post a comment you must log in.
Sergei Glushchenko (sergei.glushchenko) wrote : Posted in a previous version of this proposal

#26901

Laurynas Biveinis (laurynas-biveinis) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

> 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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) : Posted in a previous version of this proposal
review: Approve
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

> 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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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

Alexey Kopytov (akopytov) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'innobackupex'
2--- innobackupex 2012-11-07 06:44:45 +0000
3+++ innobackupex 2012-11-15 16:48:23 +0000
4@@ -719,6 +719,13 @@
5 my $file;
6 my $backup_innodb_data_file_path;
7
8+ if (has_option(\%config, $option_defaults_group, 'innodb_doublewrite_file')) {
9+ my $doublewrite_file =
10+ get_option(\%config, $option_defaults_group,
11+ 'innodb_doublewrite_file');
12+ $excluded_files = $excluded_files . '|' . $doublewrite_file;
13+ }
14+
15 # check whether files should be copied or moved to dest directory
16 my $move_or_copy_file = $move_flag ? \&move_file : \&copy_file;
17 my $move_or_copy_dir = $move_flag ?
18@@ -1727,7 +1734,7 @@
19 "innodb_log_file_size",
20 "innodb_fast_checksum",
21 "innodb_page_size",
22- "innodb_log_block_size",
23+ "innodb_log_block_size"
24 );
25
26 my $options_dump = "# This MySQL options file was generated by $innobackup_script.\n\n" .
27@@ -1741,6 +1748,12 @@
28 $options_dump .= "$option_name=$option_value\n";
29 }
30 }
31+ if (has_option(\%config,
32+ $option_defaults_group, "innodb_doublewrite_file")) {
33+ $options_dump .= "innodb_doublewrite_file=" . (split(/\/+/,
34+ get_option(\%config, $option_defaults_group,
35+ 'innodb_doublewrite_file')))[-1] . "\n";
36+ }
37
38 print FILE $options_dump;
39 close(FILE);
40
41=== modified file 'src/xtrabackup.c'
42--- src/xtrabackup.c 2012-11-08 23:42:03 +0000
43+++ src/xtrabackup.c 2012-11-15 16:48:23 +0000
44@@ -2970,6 +2970,10 @@
45 HASH_SEARCH(NAME, TABLE, FOLD, DATA, TEST)
46 #endif
47
48+#ifndef XTRADB_BASED
49+#define trx_sys_sys_space(id) (id == 0)
50+#endif
51+
52 /****************************************************************//**
53 A simple function to open or create a file.
54 @return own: handle to the file, not defined if error, error number
55@@ -3137,11 +3141,7 @@
56 info.zip_size = 0;
57 info.space_id = 0;
58
59-#ifdef XTRADB_BASED
60 if (xtrabackup_tables && (!trx_sys_sys_space(node->space->id)))
61-#else
62- if (xtrabackup_tables && (node->space->id != 0))
63-#endif
64 { /* must backup id==0 */
65 char *p;
66 int p_len, regres = REG_NOMATCH;
67@@ -3185,11 +3185,7 @@
68 }
69 }
70
71-#ifdef XTRADB_BASED
72 if (xtrabackup_tables_file && (!trx_sys_sys_space(node->space->id)))
73-#else
74- if (xtrabackup_tables_file && (node->space->id != 0))
75-#endif
76 { /* must backup id==0 */
77 xtrabackup_tables_t* table;
78 char *p;
79@@ -3231,11 +3227,7 @@
80
81 skip_filter:
82
83-#ifdef XTRADB_BASED
84 if (trx_sys_sys_space(node->space->id))
85-#else
86- if (node->space->id == 0)
87-#endif
88 {
89 char *next, *p;
90 /* system datafile "/fullpath/datafilename.ibd" or "./datafilename.ibd" */
91@@ -3396,11 +3388,7 @@
92 #endif
93 {
94 if (
95-#ifdef XTRADB_BASED
96 trx_sys_sys_space(node->space->id)
97-#else
98- node->space->id == 0
99-#endif
100 && ((offset + (IB_INT64)chunk_offset) >> page_size_shift)
101 >= FSP_EXTENT_SIZE
102 && ((offset + (IB_INT64)chunk_offset) >> page_size_shift)
103@@ -4060,11 +4048,7 @@
104 } else {
105 ptr2 = NULL;
106 }
107-#ifdef XTRADB_BASED
108 if(!trx_sys_sys_space(space->id) && ptr2)
109-#else
110- if(space->id && ptr2)
111-#endif
112 {
113 /* single table space */
114 *ptr2 = 0; /* temporary (it's my lazy..)*/
115@@ -5794,7 +5778,9 @@
116 os_file_t file = 0;
117 ulint tablespace_flags;
118
119- ut_a(dbname != NULL || space_id == 0 || space_id == ULINT_UNDEFINED);
120+ ut_a(dbname != NULL ||
121+ trx_sys_sys_space(space_id) ||
122+ space_id == ULINT_UNDEFINED);
123
124 *success = FALSE;
125
126@@ -5823,6 +5809,10 @@
127 return file;
128 }
129
130+ if (trx_sys_sys_space(space_id)) {
131+ goto found;
132+ }
133+
134 mutex_enter(&fil_system->mutex);
135 fil_space = xb_space_get_by_name(dest_space_name);
136 mutex_exit(&fil_system->mutex);
137@@ -6552,11 +6542,7 @@
138 while (space != NULL) {
139 /* treat file_per_table only */
140 if (space->purpose != FIL_TABLESPACE
141-#ifdef XTRADB_BASED
142 || trx_sys_sys_space(space->id)
143-#else
144- || space->id == 0
145-#endif
146 )
147 {
148 space = UT_LIST_GET_NEXT(space_list, space);
149@@ -7089,6 +7075,9 @@
150 printf("innodb_fast_checksum = %d\n", innobase_fast_checksum);
151 printf("innodb_page_size = %ld\n", innobase_page_size);
152 printf("innodb_log_block_size = %lu\n", innobase_log_block_size);
153+ if (innobase_doublewrite_file != NULL) {
154+ printf("innodb_doublewrite_file = %s\n", innobase_doublewrite_file);
155+ }
156 #endif
157 exit(EXIT_SUCCESS);
158 }
159
160=== added file 'test/t/ib_doublewrite.sh'
161--- test/t/ib_doublewrite.sh 1970-01-01 00:00:00 +0000
162+++ test/t/ib_doublewrite.sh 2012-11-15 16:48:23 +0000
163@@ -0,0 +1,120 @@
164+########################################################################
165+# Bug #1066843: Fix for bug #932623 does not take separate doublewrite
166+# tablespace into account
167+# Bug #1068470: XtraBackup handles separate doublewrite buffer file
168+# incorrectly
169+# We testing full and incremental backup and restore to check that
170+# separate doublewrite buffer file handled correctly
171+########################################################################
172+
173+. inc/common.sh
174+
175+if [ -z "$XTRADB_VERSION" ]; then
176+ echo "Requires XtraDB" > $SKIPPED_REASON
177+ exit $SKIPPED_EXIT_CODE
178+fi
179+
180+DBLWR=dblwr.ibd
181+start_server --innodb_file_per_table --innodb_doublewrite_file=${DBLWR}
182+load_dbase_schema incremental_sample
183+
184+# Workaround for bug #1072695
185+IB_ARGS_NO_DEFAULTS_FILE=`echo $IB_ARGS | sed -e 's/--defaults-file=[^ ]* / /'`
186+function innobackupex_no_defaults_file ()
187+{
188+ run_cmd $IB_BIN $IB_ARGS_NO_DEFAULTS_FILE $*
189+}
190+
191+echo "innodb_doublewrite_file=${DBLWR}" >>$topdir/my.cnf
192+
193+# Adding initial rows
194+vlog "Adding initial rows to database..."
195+${MYSQL} ${MYSQL_ARGS} -e "insert into test values (1, 1);" incremental_sample
196+
197+# Full backup
198+# backup root directory
199+mkdir -p $topdir/backup
200+
201+vlog "Starting backup"
202+full_backup_dir=$topdir/backup/full
203+innobackupex --no-timestamp $full_backup_dir
204+vlog "Full backup done to directory $full_backup_dir"
205+cat $full_backup_dir/backup-my.cnf
206+
207+# Changing data
208+
209+vlog "Making changes to database"
210+${MYSQL} ${MYSQL_ARGS} -e "create table t2 (a int(11) default null, number int(11) default null) engine=innodb" incremental_sample
211+${MYSQL} ${MYSQL_ARGS} -e "insert into test values (10, 1);" incremental_sample
212+${MYSQL} ${MYSQL_ARGS} -e "insert into t2 values (10, 1);" incremental_sample
213+vlog "Changes done"
214+
215+# Saving the checksum of original table
216+checksum_test_a=`checksum_table incremental_sample test`
217+checksum_t2_a=`checksum_table incremental_sample t2`
218+vlog "Table 'test' checksum is $checksum_test_a"
219+vlog "Table 't2' checksum is $checksum_t2_a"
220+
221+vlog "Making incremental backup"
222+
223+vlog "###############"
224+vlog "# INCREMENTAL #"
225+vlog "###############"
226+
227+# Incremental backup
228+inc_backup_dir=$topdir/backup/inc
229+innobackupex --no-timestamp --incremental --incremental-basedir=$full_backup_dir \
230+ $inc_backup_dir
231+vlog "Incremental backup done to directory $inc_backup_dir"
232+
233+vlog "Preparing backup"
234+# Prepare backup
235+vlog "##############"
236+vlog "# PREPARE #1 #"
237+vlog "##############"
238+innobackupex_no_defaults_file --apply-log --redo-only $full_backup_dir
239+vlog "Log applied to full backup"
240+vlog "##############"
241+vlog "# PREPARE #2 #"
242+vlog "##############"
243+innobackupex_no_defaults_file --apply-log --redo-only --incremental-dir=$inc_backup_dir \
244+ $full_backup_dir
245+vlog "Delta applied to full backup"
246+vlog "##############"
247+vlog "# PREPARE #3 #"
248+vlog "##############"
249+innobackupex_no_defaults_file --apply-log $full_backup_dir
250+vlog "Data prepared for restore"
251+
252+# Destroying mysql data
253+stop_server
254+rm -rf $mysql_datadir/*
255+vlog "Data destroyed"
256+
257+# Restore backup
258+vlog "Copying files"
259+vlog "###########"
260+vlog "# RESTORE #"
261+vlog "###########"
262+innobackupex --copy-back $full_backup_dir
263+vlog "Data restored"
264+
265+start_server --innodb_file_per_table --innodb_doublewrite_file=${DBLWR}
266+
267+vlog "Checking checksums"
268+checksum_test_b=`checksum_table incremental_sample test`
269+checksum_t2_b=`checksum_table incremental_sample t2`
270+
271+if [ "$checksum_test_a" != "$checksum_test_b" ]
272+then
273+ vlog "Checksums for table 'test' are not equal"
274+ exit -1
275+fi
276+
277+if [ "$checksum_t2_a" != "$checksum_t2_b" ]
278+then
279+ vlog "Checksums for table 't2' are not equal"
280+ exit -1
281+fi
282+
283+vlog "Checksums are OK"

Subscribers

People subscribed via source and target branches