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

[Vlad Lesin]

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/

[Alexey Kopytov (akopytov)]

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)]

> 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)]

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)]

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)]

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)]

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)]

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.

[Alexey Kopytov (akopytov)]

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

[Alexey Kopytov (akopytov)]

review: Approve

=== modified file 'src/xtrabackup.cc'
--- src/xtrabackup.cc	2013-11-12 08:56:58 +0000
+++ src/xtrabackup.cc	2013-11-13 23:52:00 +0000
@@ -5993,10 +5993,15 @@
 
         if (!xtrabackup_prepare &&
             (innobase_log_arch_dir || xtrabackup_archived_to_lsn)) {
-		msg("xtrabackup: error: "
-		    "--archived-logs-dir and --to-archived-lsn can be used "
-		    "only with --prepare\n");
-		exit(EXIT_FAILURE);
+		/*
+		  Default my.cnf can contain innobase_log_arch_dir option set for server,
+                  reset it to allow backup.
+		*/
+		innobase_log_arch_dir= NULL;
+		xtrabackup_archived_to_lsn= 0;
+		msg("xtrabackup: warning: "
+		    "as --innodb-log-arch-dir and --to-archived-lsn can be used "
+		    "only with --prepare they will be reset\n");
         }
 
 	/* cannot execute both for now */
 
=== added file 'test/t/xb_bug_1227240.sh'
--- test/t/xb_bug_1227240.sh	1970-01-01 00:00:00 +0000
+++ test/t/xb_bug_1227240.sh	2013-11-13 23:52:00 +0000
@@ -0,0 +1,39 @@
+. inc/common.sh
+# If server config file contains innodb-log-arch-dir option
+# then "xtrabackup --backup" fails as it reads this option
+# from server config file and it was supposed innodb-log-arch-dir
+# option can be used only with --prepare for xtrabackup.
+# Currently this limitation is removed and only warning outputs
+# when attempt of using innodb-log-arch-dir without --prepare
+# takes place.
+
+require_xtradb
+# The xtrabackup build and binary names are choosen automatically
+# corresponding to server version except the case when
+# build name is set explicitly with certain command line option.
+# If such option is not used the below string not only checks the
+# server version but if the check passed this means that the
+# required xtrabackup build name is set.
+require_server_version_higher_than '5.6.10'
+
+init_server_variables 1
+switch_server 1
+ARCH_LOG_DIR=$MYSQLD_VARDIR/arch_log
+BACKUP_DIR=$MYSQLD_VARDIR/backup
+reset_server_variables 1
+
+MYSQLD_EXTRA_MY_CNF_OPTS="innodb-log-archive=ON
+			  innodb-log-arch-dir=$ARCH_LOG_DIR"
+mkdir -p $ARCH_LOG_DIR $BACKUP_DIR
+
+start_server
+# If xtrabackup finished whithout error the test passed.
+xtrabackup --backup --datadir=$MYSQLD_DATADIR --target-dir=$BACKUP_DIR
+stop_server
+
+rm -rf $ARCH_LOG_DIR $BACKUP_DIR
+
+unset ARCH_LOG_DIR
+unset BACKUP_DIR
+
+exit 0