Merge lp:~vlad-lesin/percona-xtrabackup/2.1-bug-1227240 into lp:percona-xtrabackup/2.1

Proposed by Vlad Lesin on 2013-10-09
Status: Merged
Approved by: Alexey Kopytov on 2013-11-16
Approved revision: 687
Merged at revision: 694
Proposed branch: lp:~vlad-lesin/percona-xtrabackup/2.1-bug-1227240
Merge into: lp:percona-xtrabackup/2.1
Diff against target: 67 lines (+48/-4)
2 files modified
src/xtrabackup.cc (+9/-4)
test/t/xb_bug_1227240.sh (+39/-0)
To merge this branch: bzr merge lp:~vlad-lesin/percona-xtrabackup/2.1-bug-1227240
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) 2013-10-09 Approve on 2013-11-16
Review via email: mp+190068@code.launchpad.net

Description of the change

Bug #1227240 fix.

my.cnf can contain innodb_log_arch_dir option for server. xtrabackup can
use servers my.cnf. As innodb_log_arch_dir option can be used only for backup
preparing xtrabackup exits with error message in the case of backup with
innodb_log_arch_dir option. Such behavior does not allow to make backup if
default my.cnf contains innodb_log_arch_dir.

This fix ignores --innodb_log_arch_dir and --to-archived-lsn and outputs warning
if --prepare is not set.

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/456/

To post a comment you must log in.
Alexey Kopytov (akopytov) wrote :

1. Testcase?

2. The warning refers to the --archived-logs-dir option, but the current name is --innobase-log-arch-dir?

review: Needs Fixing
Vlad Lesin (vlad-lesin) wrote :

> 1. Testcase?
>
> 2. The warning refers to the --archived-logs-dir option, but the current name
> is --innobase-log-arch-dir?

Done.

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/478/

Alexey Kopytov (akopytov) wrote :

Vlad,

If you have require_* calls at the start, why do you need other checks at the end of the test (and a separate function)?

review: Needs Fixing
Vlad Lesin (vlad-lesin) wrote :

On 11/11/2013 07:32 AM, Alexey Kopytov wrote:
> Review: Needs Fixing
>
> Vlad,
>
> If you have require_* calls at the start, why do you need other checks at the end of the test (and a separate function)?
>

As the test requires archived logs enabled the check is the same as for
xb_apply_archived_logs.sh. "require_xtradb" checks only the fact that
xtrabackup is built with xtradb, but it does not examine the version of
built-in xtradb. Archived logs applying is enabled only in xtradb 5.6.

Additional function was involved for better code readability from my
point of view.

Alexey Kopytov (akopytov) wrote :

Vlad,

"require_server_version_higher_than '5.6.10'" ensures that the test is only run with xtrabackup_56 as the XtraBackup binary. If it wasn't the case, a lot of other tests would fail too. Thus checks for xtrabackup_56 (and thus, the function) are redundant, please remove them.

Vlad Lesin (vlad-lesin) wrote :

On 11/11/2013 02:05 PM, Alexey Kopytov wrote:
> Vlad,
>
> "require_server_version_higher_than '5.6.10'" ensures that the test is only run with xtrabackup_56 as the XtraBackup binary. If it wasn't the case, a lot of other tests would fail too. Thus checks for xtrabackup_56 (and thus, the function) are redundant, please remove them.
>

This is true for our default use case but this is not true for common
case. "require_server_version_higher_than" checks only server version.
If "XB_BUILD" variable is set to "autodetect"(default value) then
xtrabackup binary name is chosen based on server version(see
get_version_info()). But the default value and consequently xtrabackup
binary name can be changed with "-c" option of "run.sh".

Alexey Kopytov (akopytov) wrote :

Hi Vlad,

On Mon, 11 Nov 2013 11:03:20 -0000, Vlad Lesin wrote:
> On 11/11/2013 02:05 PM, Alexey Kopytov wrote:
>> Vlad,
>>
>> "require_server_version_higher_than '5.6.10'" ensures that the test is only run with xtrabackup_56 as the XtraBackup binary. If it wasn't the case, a lot of other tests would fail too. Thus checks for xtrabackup_56 (and thus, the function) are redundant, please remove them.
>>
>
> This is true for our default use case but this is not true for common
> case. "require_server_version_higher_than" checks only server version.
> If "XB_BUILD" variable is set to "autodetect"(default value) then
> xtrabackup binary name is chosen based on server version(see
> get_version_info()). But the default value and consequently xtrabackup
> binary name can be changed with "-c" option of "run.sh".
>

The '-c' option is only required to test "unreleased" patches and
configurations. For example, the xtrabackup binary based on
innodb51.patch is never released, innobackupex uses the xtradb51-based
binary to backup MySQL 5.1 servers.

However, we still need to support and maintain innodb51.patch in 2.1. In
2.2 all patches and the -c option will go away soon.

With that in mind, I don't see how, for example, "run.sh -c xtradb55"
against a 5.6 server would be a valid "case". It's just misusing a
feature designed for something completely different. And, as I said, a
lot of tests would fail in that case, not just this specific one.

687. By Vlad Lesin on 2013-11-13

Bug #1227240 fix.

my.cnf can contain innodb_log_arch_dir option for server. xtrabackup can
use servers my.cnf. As innodb_log_arch_dir option can be used only for backup
preparing xtrabackup exits with error message in the case of backup with
innodb_log_arch_dir option. Such behavior does not allow to make backup if
default my.cnf contains innodb_log_arch_dir.

This fix ignores --innodb_log_arch_dir and --to-archived-lsn and outputs warning
if --prepare is not set.

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 'src/xtrabackup.cc'
2--- src/xtrabackup.cc 2013-11-12 08:56:58 +0000
3+++ src/xtrabackup.cc 2013-11-13 23:52:00 +0000
4@@ -5993,10 +5993,15 @@
5
6 if (!xtrabackup_prepare &&
7 (innobase_log_arch_dir || xtrabackup_archived_to_lsn)) {
8- msg("xtrabackup: error: "
9- "--archived-logs-dir and --to-archived-lsn can be used "
10- "only with --prepare\n");
11- exit(EXIT_FAILURE);
12+ /*
13+ Default my.cnf can contain innobase_log_arch_dir option set for server,
14+ reset it to allow backup.
15+ */
16+ innobase_log_arch_dir= NULL;
17+ xtrabackup_archived_to_lsn= 0;
18+ msg("xtrabackup: warning: "
19+ "as --innodb-log-arch-dir and --to-archived-lsn can be used "
20+ "only with --prepare they will be reset\n");
21 }
22
23 /* cannot execute both for now */
24
25=== added file 'test/t/xb_bug_1227240.sh'
26--- test/t/xb_bug_1227240.sh 1970-01-01 00:00:00 +0000
27+++ test/t/xb_bug_1227240.sh 2013-11-13 23:52:00 +0000
28@@ -0,0 +1,39 @@
29+. inc/common.sh
30+# If server config file contains innodb-log-arch-dir option
31+# then "xtrabackup --backup" fails as it reads this option
32+# from server config file and it was supposed innodb-log-arch-dir
33+# option can be used only with --prepare for xtrabackup.
34+# Currently this limitation is removed and only warning outputs
35+# when attempt of using innodb-log-arch-dir without --prepare
36+# takes place.
37+
38+require_xtradb
39+# The xtrabackup build and binary names are choosen automatically
40+# corresponding to server version except the case when
41+# build name is set explicitly with certain command line option.
42+# If such option is not used the below string not only checks the
43+# server version but if the check passed this means that the
44+# required xtrabackup build name is set.
45+require_server_version_higher_than '5.6.10'
46+
47+init_server_variables 1
48+switch_server 1
49+ARCH_LOG_DIR=$MYSQLD_VARDIR/arch_log
50+BACKUP_DIR=$MYSQLD_VARDIR/backup
51+reset_server_variables 1
52+
53+MYSQLD_EXTRA_MY_CNF_OPTS="innodb-log-archive=ON
54+ innodb-log-arch-dir=$ARCH_LOG_DIR"
55+mkdir -p $ARCH_LOG_DIR $BACKUP_DIR
56+
57+start_server
58+# If xtrabackup finished whithout error the test passed.
59+xtrabackup --backup --datadir=$MYSQLD_DATADIR --target-dir=$BACKUP_DIR
60+stop_server
61+
62+rm -rf $ARCH_LOG_DIR $BACKUP_DIR
63+
64+unset ARCH_LOG_DIR
65+unset BACKUP_DIR
66+
67+exit 0

Subscribers

People subscribed via source and target branches