Merge lp:~akopytov/percona-xtrabackup/bug1130627-2.0 into lp:percona-xtrabackup/2.0

[Alexey Kopytov]

    Bug #1130627: Can't backup individual partitions

    The bug is a regression introduced in 2.0.5 by the fix for bug
    #711166. The fix introduced partition suffix ('#P#...') trimming in
    tables names before checking them against regular expressions specified
    by the --include option in innobackupex or the --tables option in
    xtrabackup. Which made impossible partial backups of partitioned tables.

    Fixed in both innobackupex and xtrabackup by first checking the table
    name without the suffix trimmed, and then repeating the check with the
    suffix trimmed if the former check fails.

    This patch also fixes bug #1131084 "Unneccessary/debug print in
    xtrabackup output" and a few other minor issues introduced by the fix
    for bug #711166.

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.0-param/357/

[Sergei Glushchenko (sergei.glushchenko)]

Alexey,

* get_table_name_with_part_suffic - is it a typo?
* innobackupex change affects only --include option (which is regex) and
  doesn't affects --databases option, i.e. with --databases option individual
  partitions still cannot be backed up
* while xtrabackup change affects both --tables and --tables-file (one with regexp, another one without)
  which looks to me as kind of inconsistency
* check_if_table_matches_regexps doesn't look like a good name since it not
  only matches against regexps, but also against table names from --tables-file
* would also be nice to have testcases which take a backup only of some partitions
  with different combinations of options

review: Needs Fixing (g2)

[Alexey Kopytov (akopytov)]

Hi Sergei,

On Thu, 07 Mar 2013 05:26:19 -0000, Sergei Glushchenko wrote:
> Review: Needs Fixing g2
>
> Alexey,
>
> * get_table_name_with_part_suffic - is it a typo?

Yes, thanks.

> * innobackupex change affects only --include option (which is regex) and
> doesn't affects --databases option, i.e. with --databases option individual
> partitions still cannot be backed up

Yes, I've made all the necessary corrections for --databases and
--tables-file, but --databases is currently broken due to bug #569387.
So the corresponding part of the test case should be uncommented once fixed.

> * while xtrabackup change affects both --tables and --tables-file (one with regexp, another one without)
> which looks to me as kind of inconsistency

Yep, it should now be consistent.

> * check_if_table_matches_regexps doesn't look like a good name since it not
> only matches against regexps, but also against table names from --tables-file

Agree, renamed to check_if_table_matches_filters()

> * would also be nice to have testcases which take a backup only of some partitions
> with different combinations of options
>

Added, with the caveat re. --databases mentioned above.

I'm currently waiting on Jenkins runs, will update the MP when they are
done.

[Alexey Kopytov (akopytov)]

Jenkins build after addressing the review comments: http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.0-param/361/

[Sergei Glushchenko (sergei.glushchenko)]

Alexey,

Everything looks good except I don't see testcase, which you mentioned as added. Probably you forget to bzr add them?

[Sergei Glushchenko (sergei.glushchenko)]

review: Needs Fixing (g2)

[Alexey Kopytov (akopytov)]

Whoops :) Good catch!

[Alexey Kopytov (akopytov)]

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.0-param/364/

[Sergei Glushchenko (sergei.glushchenko)]

Approve

review: Approve (g2)

=== modified file 'innobackupex'
--- innobackupex	2013-02-04 07:32:47 +0000
+++ innobackupex	2013-03-07 11:50:26 +0000
@@ -1137,12 +1137,14 @@
                     foreach $file (@list) {
 			next unless check_if_required($subdir, $file);
                         if($option_include) {
-                            my $table_name;
-
-                            $table_name = get_table_name($file);
-
-                            if (!("$subdir.$table_name" =~ /$option_include/)) {
-                                print STDERR "'$file' is skipped.\n";
+                            my $table = get_table_name($file);
+                            my $table_part = get_table_name_with_part_suffix($file);
+
+                            if (!("$subdir.$table_part" =~ /$option_include/ ||
+                                  "$subdir.$table" =~ /$option_include/)) {
+                                if ($print_each_file) {
+                                    print STDERR "'$file' is skipped.\n";
+                                }
                                 next;
                             }
                         }
@@ -2177,13 +2179,18 @@
             next unless check_if_required($database, $file);
 
 	    if($option_include) {
-	      my $table_name = get_table_name($file);
-	      if (!("$database.$table_name" =~ /$option_include/)) {
-	        print STDERR "$database.$file is skipped because it does not match $option_include.\n";
-	        next;
-	      }
+                my $table = get_table_name($file);
+                my $table_part = get_table_name_with_part_suffix($file);
+
+                if (!("$database.$table_part" =~ /$option_include/ ||
+                      "$database.$table" =~ /$option_include/)) {
+
+                    if ($print_each_file) {
+                        print STDERR "$database.$file is skipped because it does not match '$option_include'.\n";
+                    }
+                    next;
+                }
 	    }
-
                
             if ($print_each_file) {
                 print STDERR "$prefix Backing up file '$source_dir/$database/$file'\n";
@@ -2519,6 +2526,17 @@
 #       1 table name
 #
 sub get_table_name {
+   my $table;
+
+   $table = get_table_name_with_part_suffix($_[0]);
+   # trim partition suffix
+   $table = (split('#P#', $table))[0];
+
+   return $table;
+}
+
+# Like get_table_name(), but keeps the partition suffix if present
+sub get_table_name_with_part_suffix {
    my $table_path = shift;
    my $filename;
    my $table;
@@ -2527,8 +2545,6 @@
    $filename = (reverse(split(/\//, $table_path)))[0];
    # get name of the table by removing file suffix
    $table = (split(/\./, $filename))[0];
-   # and partition suffix
-   $table = (split('#P#', $table))[0];
 
    return $table;
 }
@@ -2546,14 +2562,16 @@
    my $db_count  = scalar keys %databases_list;
    my $tbl_count = scalar keys %table_list;
    my $table;
+   my $table_part;
 
    if ( $db_count == 0 && $tbl_count == 0 ) {
       # No databases defined with --databases option, include all databases,
       # and no tables defined with --tables-file option, include all tables.
-      return 1;
+       return 1;
    }
    else {
       if ( $table_path ) {
+         $table_part = get_table_name_with_part_suffix($table_path);
          $table = get_table_name($table_path);
       }
    }
@@ -2564,7 +2582,9 @@
          if (defined $table_path) {
             my $db_hash = $databases_list{$db};
             $db_count = keys %$db_hash;
-            if ($db_count > 0 && ! defined $databases_list{$db}->{$table}) {
+            if ($db_count > 0 &&
+                !defined $databases_list{$db}->{$table_part} &&
+                !defined $databases_list{$db}->{$table}) {
                # --databases option specified, but table is not included
                return 0;
             }
@@ -2581,7 +2601,8 @@
    # Filter for --tables-file.
    if ( $tbl_count ) {
       return 0 unless exists $table_list{$db};
-      return 0 if $table && !$table_list{$db}->{$table};
+      return 0 if $table && !$table_list{$db}->{$table_part} &&
+          !$table_list{$db}->{$table};
    }
 
    return 1;  # backup the table
 
=== modified file 'src/xtrabackup.cc'
--- src/xtrabackup.cc	2013-03-01 06:54:51 +0000
+++ src/xtrabackup.cc	2013-03-07 11:50:26 +0000
@@ -2971,6 +2971,46 @@
 }
 
 /************************************************************************
+Checks if a given table name matches any of specifications in the --tables or
+--tables-file options.
+
+@return TRUE on match. */
+static my_bool
+check_if_table_matches_filters(const char *name)
+{
+	int 			regres;
+	int 			i;
+	xtrabackup_tables_t*	table;
+
+	if (xtrabackup_tables) {
+
+		regres = REG_NOMATCH;
+
+		for (i = 0; i < tables_regex_num; i++) {
+			regres = xb_regexec(&tables_regex[i], name, 1,
+					    tables_regmatch, 0);
+			if (regres != REG_NOMATCH) {
+
+				return(TRUE);
+			}
+		}
+	}
+
+	if (xtrabackup_tables_file) {
+
+		XB_HASH_SEARCH(name_hash, tables_hash, ut_fold_string(name),
+			       table, (void) 0,
+			       !strcmp(table->name, name));
+		if (table) {
+
+			return(TRUE);
+		}
+	}
+
+	return(FALSE);
+}
+
+/************************************************************************
 Checks if a table specified as a path should be skipped from backup
 based on the --tables or --tables-file options.
 
@@ -3009,37 +3049,24 @@
 	}
 	buf[dbname_len - 1] = 0;
 
+	/* For partitioned tables first try to match against the regexp
+	without truncating the #P#... suffix so we can backup individual
+	partitions with regexps like '^test[.]t#P#p5' */
+	if (check_if_table_matches_filters(buf)) {
+
+		return(FALSE);
+	}
 	if ((eptr = strstr(buf, "#P#")) != NULL) {
+
 		*eptr = 0;
-	}
-
-	if (xtrabackup_tables) {
-		int regres = REG_NOMATCH;
-		int i;
-		for (i = 0; i < tables_regex_num; i++) {
-			regres = xb_regexec(&tables_regex[i], buf, 1,
-					    tables_regmatch, 0);
-			if (regres != REG_NOMATCH) {
-				break;
-			}
-		}
-		if (regres == REG_NOMATCH) {
-			return(TRUE);
-		}
-	}
-
-	if (xtrabackup_tables_file) {
-		xtrabackup_tables_t*	table;
-
-		XB_HASH_SEARCH(name_hash, tables_hash, ut_fold_string(buf),
-			       table, (void) 0,
-			       !strcmp(table->name, buf));
-		if (!table) {
-			return(TRUE);
-		}
-	}
-
-	return(FALSE);
+
+		if (check_if_table_matches_filters(buf)) {
+
+			return(FALSE);
+		}
+	}
+
+	return(TRUE);
 }
 
 /***********************************************************************
@@ -3313,8 +3340,7 @@
 
 	if ((!trx_sys_sys_space(node->space->id))
 		&& check_if_skip_table(node->name, "ibd")) {
-		printf("[%02u] Skipping %s.\n",
-		       thread_n, node->name);
+		msg("[%02u] Skipping %s.\n", thread_n, node->name);
 		return(FALSE);
 	}
 
@@ -4337,6 +4363,8 @@
 
 		p = xtrabackup_tables;
 		for (i=0; i < tables_regex_num; i++) {
+			int	rc;
+
 			next = strchr(p, ',');
 			ut_a(next || i == tables_regex_num - 1);
 
@@ -4344,10 +4372,18 @@
 			if (i != tables_regex_num - 1)
 				*(next - 1) = '\0';
 
-			xb_regerror(xb_regcomp(&tables_regex[i], p,
-					       REG_EXTENDED),
-				    &tables_regex[i], errbuf, sizeof(errbuf));
-			msg("xtrabackup: tables regcomp(%s): %s\n", p, errbuf);
+			rc = xb_regcomp(&tables_regex[i], p,
+					REG_EXTENDED);
+
+			if (rc) {
+
+				xb_regerror(rc, &tables_regex[i], errbuf,
+					    sizeof(errbuf));
+				msg("xtrabackup: regcomp(%s) failed: %s\n",
+				    p, errbuf);
+
+				exit(EXIT_FAILURE);
+			}
 
 			if (i != tables_regex_num - 1)
 				*(next - 1) = ',';
@@ -4394,9 +4430,6 @@
 
 			HASH_INSERT(xtrabackup_tables_t, name_hash, tables_hash,
 					ut_fold_string(table->name), table);
-
-			msg("xtrabackup: table '%s' is registered to the "
-			    "list.\n", table->name);
 		}
 	}
 }
 
=== added file 'test/t/bug1130627.sh'
--- test/t/bug1130627.sh	1970-01-01 00:00:00 +0000
+++ test/t/bug1130627.sh	2013-03-07 11:50:26 +0000
@@ -0,0 +1,74 @@
+########################################################################
+# Bug #1130627: Can't backup individual partitions
+########################################################################
+
+. inc/common.sh
+. inc/ib_part.sh
+
+start_server --innodb_file_per_table
+
+require_partitioning
+
+run_cmd $MYSQL $MYSQL_ARGS test <<EOF
+CREATE TABLE t_innodb (
+  i INT NOT NULL AUTO_INCREMENT,
+  c CHAR(10) DEFAULT NULL,
+  PRIMARY KEY (i)
+) ENGINE=InnoDB
+/*!50100 PARTITION BY HASH (i)
+PARTITIONS 10 */;
+
+CREATE TABLE t_myisam (
+  i INT NOT NULL AUTO_INCREMENT,
+  c CHAR(10) DEFAULT NULL,
+  PRIMARY KEY (i)
+) ENGINE=MyISAM
+/*!50100 PARTITION BY HASH (i)
+PARTITIONS 10 */;
+EOF
+
+# Force a checkpoint
+stop_server
+start_server --innodb_file_per_table
+
+# Test that specifying partitions with --include works
+innobackupex --no-timestamp --include='^test.*#P#p5' $topdir/backup
+innobackupex --apply-log $topdir/backup
+
+file_cnt=`ls $topdir/backup/test | wc -l`
+
+test "$file_cnt" -eq 3
+
+test -f $topdir/backup/test/t_innodb#P#p5.ibd
+test -f $topdir/backup/test/t_myisam#P#p5.MYD
+test -f $topdir/backup/test/t_myisam#P#p5.MYI
+
+rm -rf $topdir/backup
+
+# Test that specifying partitions with --databases works
+# TODO: uncomment when bug #569387 is fixed
+
+# innobackupex --no-timestamp \
+#     --databases='test.t_myisam#P#p5 test.t_innodb#P#p5' $topdir/backup
+# innobackupex --apply-log $topdir/backup
+
+# test "$file_cnt" -eq 3
+# test -f $topdir/backup/test/t_innodb#P#p5.ibd
+# test -f $topdir/backup/test/t_myisam#P#p5.MYD
+# test -f $topdir/backup/test/t_myisam#P#p5.MYI
+
+# rm -rf $topdir/backup
+
+# Test that specifying partitions with --tables-file works
+cat >$topdir/tables_file <<EOF
+test.t_myisam#P#p5
+test.t_innodb#P#p5
+EOF
+innobackupex --no-timestamp --tables-file=$topdir/tables_file $topdir/backup
+innobackupex --apply-log $topdir/backup
+
+test "$file_cnt" -eq 3
+test -f $topdir/backup/test/t_myisam#P#p5.MYD
+test -f $topdir/backup/test/t_myisam#P#p5.MYI
+
+rm -rf $topdir/backup