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

[Sergei Glushchenko]

Bug 543134: LRU dump/restore support in XtraBackup
Support is done by copying file ib_lru_dump inside of innobackupex script.
Both for local-copy and stream modes this file is copied outside of
FLUSH TABLES WITH READ LOCK section. For rsync mode it is copied along
with other files by single rsync call.

[Sergei Glushchenko (sergei.glushchenko)]

Jenkins build
http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-param/118/

[Alexey Kopytov (akopytov)]

Hi Sergei,

On 27.01.12 10:12, Sergei Glushchenko wrote:
> === modified file 'innobackupex'
> --- innobackupex 2012-01-23 21:39:00 +0000
> +++ innobackupex 2012-01-27 06:10:35 +0000
> @@ -423,6 +423,24 @@
> # Close the DB connection
> mysql_close();
>
> + # copy ib_lru_dump
> + if (-e "$orig_datadir/ib_lru_dump") {
> + if ($option_remote_host) {
> + print STDERR "$prefix Backing up file '$src_name'\n";

I think it should be $orig_datadir/ib_lru_dump instead of $src_name on
the above line. But I don't think you really need this code, see the
next comment.

> + } elsif (!$option_rsync) {
> + my $src_name = escape_path("$orig_datadir/ib_lru_dump");
> + my $dst_name = escape_path("$backup_dir/ib_lru_dump");
> + system("$CP_CMD \"$src_name\" \"$dst_name\"")
> + and Die "Failed to copy file 'ib_lru_dump': $!";
> + }
> + }

Why not just add it to the regexp in backup_files() so it's copied as a
regular data file, rather than a special xtrabackup meta info file?

>
> + # copy ib_lru_dump to datadir
> + if (-e "$backup_dir/ib_lru_dump") {
> + print STDERR "$prefix Copying file '$backup_dir/ib_lru_dump'\n";
> + $src_name = escape_path("$backup_dir/ib_lru_dump");
> + $dst_name = escape_path("$orig_datadir");
> + system("$CP_CMD \"$src_name\" \"$dst_name\"")
> + and Die "Failed to copy file 'ib_lru_dump': $!";
> + }
> +

Is it really needed? I think it would be copied by copy_back() even
without this code. Also not that copy_back() has been changed
significantly with the fix for bug #759701, which got merged to main
trees today.

> if ($option_rsync) {
> + if (-e "$source_dir/ib_lru_dump") {
> + print RSYNC "ib_lru_dump\n";
> + if (!$prep_mode) {
> + $rsync_files_hash{"ib_lru_dump"} = 1;
> + }
> + }

You won't need this either with the change in backup_files() I suggested
above.

[Alexey Kopytov (akopytov)]

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

There is some meaning in the way I'm copying ib_lru_dump. Since this file is not a table, we couldn't worry about it's consistency and we could copy it without locking all tables. I'm not sure if it give us some advantages. So correct me if it not.
If it is acceptable to copy ib_lru_dump with tables locked then solution would be bit different and much easier.
I also noticed that parts of innobackupex which I touched in this branch changed significantly. It would probably better to rebase this branch on more recent revision.

[Sergei Glushchenko (sergei.glushchenko)]

review: Needs Information

[Sergei Glushchenko (sergei.glushchenko)]

Alexey,

Anyway regexp /\.(frm)|(MYD)|(MYI)|(MRG)|(TRG)|(TRN)|(ARM)|(ARZ)|(CSM)|(CSV)|(opt)|(par)$/ is not the best place to handle ib_lru_dump. It's because of 2 things:

1. ib_lru_dump placed in mysql datadir, so if we want it to be handled we must change

    if ($database eq '.' || $database eq '..') { next; }

to

    if ($database eq '..') { next; }

2. I think ib_lru_dump shouldn't go thru check_if_required (filter by --databases, --tables-file)

What do you think about it?

[Alexey Kopytov (akopytov)]

On 31.01.12 15:44, Sergei Glushchenko wrote:
> There is some meaning in the way I'm copying ib_lru_dump. Since this file is not a table, we couldn't worry about it's consistency and we could copy it without locking all tables. I'm not sure if it give us some advantages. So correct me if it not.
> If it is acceptable to copy ib_lru_dump with tables locked then solution would be bit different and much easier.

Right, I forgot copying it outside FTWRL is required by the bug report.

OK, let's do it outside then.

Few minor notes on test cases:

- You can check if you are running against Percona Server (i.e. require
XtraDB), rather than issue an SQL comment with "set +e" and then
checking if file exists.

- you can also check for XtraDB before calling 'init' and 'run_mysqld'
to save some time when the test should be skipped.

- same goes for rsync: you should normally check if the test should be
skipped as early as possible

- please use $TAR instead of calling tar directly, so that we use gnutar
where applicable.

- I think you can use:

if $TAR itf $topdir/backup/stream.tar | grep ib_lru_dump ; then

instead of:

if [ "`tar itf $topdir/backup/stream.tar | grep ib_lru_dump | wc -l`" ==
"1" ] ; then

- let's follow some common conventions for test headers, see for example
bug759225.sh or bug514068.sh.

- 'backup' is not a verb, but 'back' is. so it should be 'backed up'
rather than 'backuped'.

[Alexey Kopytov (akopytov)]

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

Alexey,

> - You can check if you are running against Percona Server (i.e. require
> XtraDB), rather than issue an SQL comment with "set +e" and then
> checking if file exists.
>
> - you can also check for XtraDB before calling 'init' and 'run_mysqld'
> to save some time when the test should be skipped.
>
> - same goes for rsync: you should normally check if the test should be
> skipped as early as possible
>
> - please use $TAR instead of calling tar directly, so that we use gnutar
> where applicable.
>
> - I think you can use:
>
> if $TAR itf $topdir/backup/stream.tar | grep ib_lru_dump ; then
>
> instead of:
>
> if [ "`tar itf $topdir/backup/stream.tar | grep ib_lru_dump | wc -l`" ==
> "1" ] ; then
>
> - let's follow some common conventions for test headers, see for example
> bug759225.sh or bug514068.sh.
>
> - 'backup' is not a verb, but 'back' is. so it should be 'backed up'
> rather than 'backuped'.

^ All those thing are fixed.

Removed block of code which from copy-back, as it was not necessary.

Branch based on revision of xtrabackup without kewpie.

[Sergei Glushchenko (sergei.glushchenko)]

Jenkins build:
http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-param/128/

[Alexey Kopytov (akopytov)]

OK to merge, thanks!

review: Approve

=== modified file 'innobackupex'
--- innobackupex	2012-02-06 10:10:40 +0000
+++ innobackupex	2012-02-06 12:52:23 +0000
@@ -430,6 +430,24 @@
     # Close the DB connection
     mysql_close();
 
+    # copy ib_lru_dump
+    if (-e "$orig_datadir/ib_lru_dump") {
+        if ($option_remote_host) {
+            print STDERR "$prefix Backing up file 'ib_lru_dump'\n";
+            system("scp $option_scp_opt '$orig_datadir/ib_lru_dump' '$option_remote_host:$backup_dir/ib_lru_dump'")
+                and Die "Failed to scp file 'ib_lru_dump': $!";
+        } elsif ($option_stream) {
+            print STDERR "$prefix Backing up as tar stream 'ib_lru_dump'\n";
+            system("cd $orig_datadir; tar chf - ib_lru_dump")
+                and Die "Failed to stream 'ib_lru_dump': $!";
+        } elsif (!$option_rsync) {
+            my $src_name = escape_path("$orig_datadir/ib_lru_dump");
+            my $dst_name = escape_path("$backup_dir/ib_lru_dump");
+            system("$CP_CMD \"$src_name\" \"$dst_name\"")
+                and Die "Failed to copy file 'ib_lru_dump': $!";
+        }
+    }
+
     if ($option_remote_host) {
         system("scp $option_scp_opt '$tmp_logfile' '$option_remote_host:$backup_dir/xtrabackup_logfile'")
             and Die "Failed to scp file '$option_remote_host:$backup_dir/xtrabackup_logfile': $!";
@@ -2108,6 +2126,12 @@
     closedir(DIR);
 
     if ($option_rsync) {
+        if (-e "$source_dir/ib_lru_dump") {
+            print RSYNC "ib_lru_dump\n";
+            if (!$prep_mode) {
+                $rsync_files_hash{"ib_lru_dump"} = 1;
+            }
+        }
 	close(RSYNC);
 
 	# do the actual rsync now

=== added file 'test/t/ib_lru_dump_basic.sh'
--- test/t/ib_lru_dump_basic.sh	1970-01-01 00:00:00 +0000
+++ test/t/ib_lru_dump_basic.sh	2012-02-06 12:52:23 +0000
@@ -0,0 +1,37 @@
+########################################################################
+# Bug #543134: Missing support for LRU dump/restore
+########################################################################
+
+. inc/common.sh
+
+if [ -z "$XTRADB_VERSION" ]; then
+    echo "Requires XtraDB" > $SKIPPED_REASON
+    exit $SKIPPED_EXIT_CODE
+fi
+
+init
+run_mysqld
+
+# produce ib_lru_dump
+${MYSQL} ${MYSQL_ARGS} -e "select * from information_schema.XTRADB_ADMIN_COMMAND /*!XTRA_LRU_DUMP*/;"
+
+# take a backup
+innobackupex --no-timestamp $topdir/backup
+
+if [ -f $topdir/backup/ib_lru_dump ] ; then
+    vlog "LRU dump has been backed up"
+else
+    vlog "LRU dump has not been backed up"
+    exit -1
+fi
+
+# restore from backup
+rm -rf $mysql_datadir/*
+innobackupex --copy-back $topdir/backup
+
+if [ -f $mysql_datadir/ib_lru_dump ] ; then
+    vlog "LRU dump has been restored"
+else
+    vlog "LRU dump has not been restored"
+    exit -1
+fi

=== added file 'test/t/ib_lru_dump_rsync.sh'
--- test/t/ib_lru_dump_rsync.sh	1970-01-01 00:00:00 +0000
+++ test/t/ib_lru_dump_rsync.sh	2012-02-06 12:52:23 +0000
@@ -0,0 +1,31 @@
+########################################################################
+# Bug #543134: Missing support for LRU dump/restore
+########################################################################
+
+. inc/common.sh
+
+if [ -z "$XTRADB_VERSION" ]; then
+    echo "Requires XtraDB" > $SKIPPED_REASON
+    exit $SKIPPED_EXIT_CODE
+fi
+
+if ! which rsync > /dev/null 2>&1 ; then
+    echo "Requires rsync to be installed" > $SKIPPED_REASON
+    exit $SKIPPED_EXIT_CODE
+fi
+
+init
+run_mysqld
+
+# produce ib_lru_dump
+${MYSQL} ${MYSQL_ARGS} -e "select * from information_schema.XTRADB_ADMIN_COMMAND /*!XTRA_LRU_DUMP*/;"
+
+# take a backup with rsync mode
+innobackupex --rsync --no-timestamp $topdir/backup
+
+if [ -f $topdir/backup/ib_lru_dump ] ; then
+    vlog "LRU dump has been backed up"
+else
+    vlog "LRU dump has not been backed up"
+    exit -1
+fi

=== added file 'test/t/ib_lru_dump_stream.sh'
--- test/t/ib_lru_dump_stream.sh	1970-01-01 00:00:00 +0000
+++ test/t/ib_lru_dump_stream.sh	2012-02-06 12:52:23 +0000
@@ -0,0 +1,27 @@
+########################################################################
+# Bug #543134: Missing support for LRU dump/restore
+########################################################################
+
+. inc/common.sh
+
+if [ -z "$XTRADB_VERSION" ]; then
+    echo "Requires XtraDB" > $SKIPPED_REASON
+    exit $SKIPPED_EXIT_CODE
+fi
+
+init
+run_mysqld
+
+# produce ib_lru_dump
+${MYSQL} ${MYSQL_ARGS} -e "select * from information_schema.XTRADB_ADMIN_COMMAND /*!XTRA_LRU_DUMP*/;"
+
+# take a backup with stream mode
+mkdir -p $topdir/backup
+innobackupex --stream=tar $topdir/backup > $topdir/backup/stream.tar
+
+if $TAR itf $topdir/backup/stream.tar | grep ib_lru_dump ; then
+    vlog "LRU dump has been backed up"
+else
+    vlog "LRU dump has not been backed up"
+    exit -1
+fi