Merge lp:~percona-dev/percona-xtrabackup/bug723097 into lp:percona-xtrabackup/2.0

Proposed by Valentine Gostev
Status: Merged
Approved by: Alexey Kopytov
Approved revision: 243
Merged at revision: 253
Proposed branch: lp:~percona-dev/percona-xtrabackup/bug723097
Merge into: lp:percona-xtrabackup/2.0
Diff against target: 92 lines (+47/-4)
3 files modified
test/t/bug723097.sh (+38/-0)
test/t/bug723097.sql (+5/-0)
utils/build.sh (+4/-4)
To merge this branch: bzr merge lp:~percona-dev/percona-xtrabackup/bug723097
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Approve
Review via email: mp+58780@code.launchpad.net

This proposal supersedes a proposal from 2011-04-01.

Description of the change

Added fix and test for bug 723097

1. Removed unnecessary lines from SQL dump
2. Added checksum validation

To post a comment you must log in.
Revision history for this message
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

The test case passes even on an unmodifed xtrabackup.

review: Needs Fixing
Revision history for this message
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

Valentine,

Some comments after analyzing the test case more closely:

- The test does not really involve any non-latin characters and thus, does not really test the problem. It's easy to verify by looking at the hex dump for the .sql file:

$ hexdump -C experimental/bug723097.sql | grep INSERT
000004c0 2a 21 34 30 30 30 30 20 41 4c 54 45 52 20 54 41 |*!40000 ALTER TA|
000004d0 42 4c 45 20 60 6d 65 73 73 61 67 65 73 60 20 44 |BLE `messages` D|
000004e0 49 53 41 42 4c 45 20 4b 45 59 53 20 2a 2f 3b 0a |ISABLE KEYS */;.|
000004f0 49 4e 53 45 52 54 20 49 4e 54 4f 20 60 6d 65 73 |INSERT INTO `mes|
00000500 73 61 67 65 73 60 20 56 41 4c 55 45 53 20 28 27 |sages` VALUES ('|
00000510 3f 3f 3f 3f 20 3f 3f 3f c2 b1 20 3f 3f 3f 3f 3f |???? ???.. ?????|
00000520 3f 3f 3f 3f c2 b1 27 29 3b 0a 2f 2a 21 34 30 30 |????..');./*!400|

So those '?' characters are literally question marks (3f).

That's why the test cases passes even on an unmodified xtrabackup binary.

- I don't think we need a 49-line .sql file just to create 1-row table.

- I don't think the root problem has anything to do with race conditions and hence requires some parallel background activities. This has to be investigated further. If it turns out we don't really need to run any background activities, the test case has to be added to the main 'suite' rather than 'experimental'. Otherwise we have to think about implementing some kind of synchronization facility for xtrabackup tests, because the number tests which are experimental simply due to the lack of such facility is growing.

- The query following query: "insert into messages select * from messages" doubles the test table size and is run in a tight loop during the test. On my laptop it resulted in ~60 MB table during the first few seconds of the test run. Does this test really have to consume that much resources?

Revision history for this message
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

Valentine,

1. I think "SET NAMES utf8" must also be issued before loading the data. You can put it into the same .sql file.

2. I don't think you need to create a separate database for this. The default one ('test') will do and does not have to be created explicitly.

review: Needs Fixing
Revision history for this message
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

Please also note the conflict in build.sh.

review: Needs Fixing
Revision history for this message
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

Sorry, please disregard the comment about the conflict, I was reviewing the old MP version.

Revision history for this message
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

Copying the comments from the older MP:

Valentine,

1. I think "SET NAMES utf8" must also be issued before loading the data. You can put it into the same .sql file.

2. I don't think you need to create a separate database for this. The default one ('test') will do and does not have to be created explicitly.

review: Needs Fixing
Revision history for this message
Alexey Kopytov (akopytov) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'test/t/bug723097.sh'
2--- test/t/bug723097.sh 1970-01-01 00:00:00 +0000
3+++ test/t/bug723097.sh 2011-04-22 02:57:28 +0000
4@@ -0,0 +1,38 @@
5+. inc/common.sh
6+
7+init
8+run_mysqld --innodb_file_per_table
9+
10+vlog "Loading data from sql file"
11+run_cmd ${MYSQL} ${MYSQL_ARGS} test < t/bug723097.sql
12+
13+vlog "Saving checksum"
14+checksum_a=`${MYSQL} ${MYSQL_ARGS} -Ns -e "CHECKSUM TABLE messages" test | awk {'print $2'}`
15+vlog "Checksum before is $checksum_a"
16+
17+vlog "Creating backup directory"
18+mkdir -p $topdir/data/full
19+vlog "Starting backup"
20+xtrabackup --datadir=$mysql_datadir --backup --target-dir=$topdir/data/full
21+vlog "Backup is done"
22+xtrabackup --datadir=$mysql_datadir --prepare --target-dir=$topdir/data/full
23+vlog "Data prepared fo restore"
24+stop_mysqld
25+
26+cd $topdir/data/full
27+cp -r * $mysql_datadir
28+cd -
29+run_mysqld --innodb_file_per_table
30+checksum_b=`${MYSQL} ${MYSQL_ARGS} -Ns -e "CHECKSUM TABLE messages" test | awk {'print $2'}`
31+vlog "Checksum after is $checksum_b"
32+stop_mysqld
33+
34+if [ $checksum_a -eq $checksum_b ]
35+then
36+ vlog "Checksums are Ok"
37+else
38+ vlog "Checksums are not equal"
39+ exit -1
40+fi
41+
42+clean
43
44=== added file 'test/t/bug723097.sql'
45--- test/t/bug723097.sql 1970-01-01 00:00:00 +0000
46+++ test/t/bug723097.sql 2011-04-22 02:57:28 +0000
47@@ -0,0 +1,5 @@
48+SET NAMES utf8;
49+CREATE TABLE `messages` (
50+ `a` varchar(20) DEFAULT NULL
51+) ENGINE=InnoDB DEFAULT CHARSET=greek;
52+INSERT INTO `messages` VALUES ('πτήσης');
53
54=== modified file 'utils/build.sh'
55--- utils/build.sh 2011-04-21 06:40:52 +0000
56+++ utils/build.sh 2011-04-22 02:57:28 +0000
57@@ -160,7 +160,7 @@
58 --with-plugins=innobase \
59 --with-zlib-dir=bundled \
60 --enable-shared \
61- --with-extra-charsets=complex"
62+ --with-extra-charsets=all"
63
64 build_all
65 ;;
66@@ -176,7 +176,7 @@
67 -DWITH_INNOBASE_STORAGE_ENGINE=ON \
68 -DWITH_PARTITION_STORAGE_ENGINE=ON \
69 -DWITH_ZLIB=bundled \
70- -DWITH_EXTRA_CHARSETS=complex \
71+ -DWITH_EXTRA_CHARSETS=all \
72 -DENABLE_DTRACE=OFF"
73
74 build_all
75@@ -193,7 +193,7 @@
76 --with-plugins=innodb_plugin \
77 --with-zlib-dir=bundled \
78 --enable-shared \
79- --with-extra-charsets=complex"
80+ --with-extra-charsets=all"
81 if [ "`uname -s`" = "Linux" ]
82 then
83 configure_cmd="LIBS=-lrt $configure_cmd"
84@@ -240,7 +240,7 @@
85 -DWITH_INNOBASE_STORAGE_ENGINE=ON \
86 -DWITH_PARTITION_STORAGE_ENGINE=ON \
87 -DWITH_ZLIB=bundled \
88- -DWITH_EXTRA_CHARSETS=complex \
89+ -DWITH_EXTRA_CHARSETS=all \
90 -DENABLE_DTRACE=OFF"
91 if [ "`uname -s`" = "Linux" ]
92 then

Subscribers

People subscribed via source and target branches