Merge lp:~percona-toolkit-dev/percona-toolkit/pt-online-schema-change-doesnt-work-with-HASH-indexes-1340728 into lp:~percona-toolkit-dev/percona-toolkit/release-2.2.10

[Frank Cizmich]

Intended to make pt-osc work with engines that allow a HASH index (MEMORY , NDB) . When issued the EXPLAIN query, this type of index sometimes doesn't select a primary key even though it's available.
This fix makes the --nocheck-plan option skip *all* checks on the key chosen by EXPLAIN.

[Daniel Nichter (daniel-nichter)]

We talked on IRC about some changes. I'll set this as "Needs Fixing" until then.

review: Needs Fixing

[lp:~percona-toolkit-dev/percona-toolkit/pt-online-schema-change-doesnt-work-with-HASH-indexes-1340728]

620. By Frank Cizmich on 2014-07-31

Skipped another plan check. Skipped running explain when unnecessary. Added test. Fixed test that set check-plan off by default

[Daniel Nichter (daniel-nichter)]

Code is good, let's just tweak the test a little.

review: Needs Fixing

[lp:~percona-toolkit-dev/percona-toolkit/pt-online-schema-change-doesnt-work-with-HASH-indexes-1340728]

621. By Frank Cizmich on 2014-08-05

used output instead of full_output. merged bug tests into bugs.t

[Daniel Nichter (daniel-nichter)]

review: Approve

=== modified file 'bin/pt-online-schema-change'
--- bin/pt-online-schema-change	2014-07-04 17:16:08 +0000
+++ bin/pt-online-schema-change	2014-08-05 15:51:35 +0000
@@ -8908,28 +8908,31 @@
          # boundary sql.  This check applies to the next nibble.  So if
          # the current nibble number is 5, then nibble 5 is already done
          # and we're checking nibble number 6.
-         my $expl = explain_statement(
-            tbl  => $tbl,
-            sth  => $sth->{explain_upper_boundary},
-            vals => [ @{$boundary->{lower}}, $nibble_iter->limit() ],
-         );
-         if (lc($expl->{key} || '') ne lc($nibble_iter->nibble_index() || '')) {
-            my $msg
-               = "Aborting copying table $tbl->{name} at chunk "
-               . ($nibble_iter->nibble_number() + 1)
-               . " because it is not safe to ascend.  Chunking should "
-               . "use the "
-               . ($nibble_iter->nibble_index() || '?')
-               . " index, but MySQL EXPLAIN reports that "
-               . ($expl->{key} ? "the $expl->{key}" : "no")
-               . " index will be used for "
-               . $sth->{upper_boundary}->{Statement}
-               . " with values "
-               . join(", ", map { defined $_ ? $_ : "NULL" }
-                      (@{$boundary->{lower}}, $nibble_iter->limit()))
-               . "\n";
-            die ts($msg);
-         } 
+         # Skip if --nocheck-plan   See: https://bugs.launchpad.net/percona-toolkit/+bug/1340728
+         if ( $o->get('check-plan') ) {
+            my $expl = explain_statement(
+               tbl  => $tbl,
+               sth  => $sth->{explain_upper_boundary},
+               vals => [ @{$boundary->{lower}}, $nibble_iter->limit() ],
+            );
+            if ( lc($expl->{key} || '') ne lc($nibble_iter->nibble_index() || '') ) {
+               my $msg
+                  = "Aborting copying table $tbl->{name} at chunk "
+                  . ($nibble_iter->nibble_number() + 1)
+                  . " because it is not safe to ascend.  Chunking should "
+                  . "use the "
+                  . ($nibble_iter->nibble_index() || '?')
+                  . " index, but MySQL EXPLAIN reports that "
+                  . ($expl->{key} ? "the $expl->{key}" : "no")
+                  . " index will be used for "
+                  . $sth->{upper_boundary}->{Statement}
+                  . " with values "
+                  . join(", ", map { defined $_ ? $_ : "NULL" }
+                         (@{$boundary->{lower}}, $nibble_iter->limit()))
+                  . "\n";
+               die ts($msg);
+            } 
+         }
 
          # Once nibbling begins for a table, control does not return to this
          # tool until nibbling is done because, as noted above, all work is
@@ -9531,15 +9534,17 @@
       vals => [ @{$boundary->{lower}}, @{$boundary->{upper}} ],
    );
 
-   # Ensure that MySQL is using the chunk index if the table is being chunked.
-   if ( !$nibble_iter->one_nibble()
-        && lc($expl->{key} || '') ne lc($nibble_iter->nibble_index() || '') )
-   {
-      die ts("Error copying rows at chunk " . $nibble_iter->nibble_number()
-         . " of $tbl->{db}.$tbl->{tbl} because MySQL chose "
-         . ($expl->{key} ? "the $expl->{key}" : "no") . " index "
-         . " instead of the " . $nibble_iter->nibble_index() . "index.\n");
-   }
+   # Ensure that MySQL is using the chunk index if the table is being chunked. 
+   # Skip if --nocheck-plan   See: https://bugs.launchpad.net/percona-toolkit/+bug/1340728
+      if ( !$nibble_iter->one_nibble()
+           && lc($expl->{key} || '') ne lc($nibble_iter->nibble_index() || '') 
+           && $o->get('check-plan') )
+      {
+         die ts("Error copying rows at chunk " . $nibble_iter->nibble_number()
+            . " of $tbl->{db}.$tbl->{tbl} because MySQL chose "
+            . ($expl->{key} ? "the $expl->{key}" : "no") . " index "
+            . " instead of the " . $nibble_iter->nibble_index() . "index.\n");
+      }
 
    # Ensure that the chunk isn't too large if there's a --chunk-size-limit.
    # If single-chunking the table, this has already been checked, so it
@@ -9564,11 +9569,13 @@
       }
    }
 
-   # Ensure that MySQL is still using the entire index.
+   # Ensure that MySQL is still using the entire index. 
    # https://bugs.launchpad.net/percona-toolkit/+bug/1010232
+   # Skip if --nocheck-plan   See: https://bugs.launchpad.net/percona-toolkit/+bug/1340728
    if ( !$nibble_iter->one_nibble()
         && $tbl->{key_len}
-        && ($expl->{key_len} || 0) < $tbl->{key_len} )
+        && ($expl->{key_len} || 0) < $tbl->{key_len} 
+        && $o->get('check-plan') )
    {
       die ts("Error copying rows at chunk " . $nibble_iter->nibble_number()
          . " of $tbl->{db}.$tbl->{tbl} because MySQL used "

=== removed file 't/pt-online-schema-change/bug-1315130.t'
--- t/pt-online-schema-change/bug-1315130.t	2014-06-20 21:31:30 +0000
+++ t/pt-online-schema-change/bug-1315130.t	1970-01-01 00:00:00 +0000
@@ -1,78 +0,0 @@
-#!/usr/bin/env perl
-
-BEGIN {
-   die "The PERCONA_TOOLKIT_BRANCH environment variable is not set.\n"
-      unless $ENV{PERCONA_TOOLKIT_BRANCH} && -d $ENV{PERCONA_TOOLKIT_BRANCH};
-   unshift @INC, "$ENV{PERCONA_TOOLKIT_BRANCH}/lib";
-};
-
-use strict;
-use warnings FATAL => 'all';
-use English qw(-no_match_vars);
-use Test::More;
-use Time::HiRes qw(sleep);
-
-$ENV{PTTEST_FAKE_TS} = 1;
-$ENV{PERCONA_TOOLKIT_TEST_USE_DSN_NAMES} = 1;
-
-use PerconaTest;
-use Sandbox;
-require "$trunk/bin/pt-online-schema-change";
-require VersionParser;
-
-use Data::Dumper;
-$Data::Dumper::Indent    = 1;
-$Data::Dumper::Sortkeys  = 1;
-$Data::Dumper::Quotekeys = 0;
-
-my $dp         = new DSNParser(opts=>$dsn_opts);
-my $sb         = new Sandbox(basedir => '/tmp', DSNParser => $dp);
-my $master_dbh = $sb->get_dbh_for('master');
-my $slave_dbh  = $sb->get_dbh_for('slave1');
-
-if ( !$master_dbh ) {
-   plan skip_all => 'Cannot connect to sandbox master';
-}
-elsif ( !$slave_dbh ) {
-   plan skip_all => 'Cannot connect to sandbox slave';
-}
-
-my $q      = new Quoter();
-my $tp     = new TableParser(Quoter => $q);
-my @args   = qw(--set-vars innodb_lock_wait_timeout=3);
-my $output = "";
-my $dsn    = "h=127.1,P=12345,u=msandbox,p=msandbox";
-my $exit   = 0;
-my $sample = "t/pt-online-schema-change/samples";
-my $rows;
-
-
-# #############################################################################
-# Issue 1315130
-# Failed to detect child tables in other schema, and falsely identified
-# child tables in own schema
-# #############################################################################
-
-$sb->load_file('master', "$sample/bug-1315130_cleanup.sql");
-$sb->load_file('master', "$sample/bug-1315130.sql");
-($output, $exit) = full_output(
-   sub { pt_online_schema_change::main(@args, "$dsn,D=bug_1315130_a,t=parent_table",
-         '--dry-run', 
-         '--alter', "add column c varchar(16)",
-         '--alter-foreign-keys-method', 'auto'),
-      },
-);
-print STDERR "[$output]\n";
-   like(
-         $output,
-         qr/Child tables:\s*`bug_1315130_a`\.`child_table_in_same_schema` \(approx\. 1 rows\)\s*`bug_1315130_b`\.`child_table_in_second_schema` \(approx\. 1 rows\)[^`]*?Will/s,
-         "Correctly identify child tables from other schemas and ignores tables from same schema referencig same named parent in other schema.",
-   );
-# clear databases with their foreign keys
-$sb->load_file('master', "$sample/bug-1315130_cleanup.sql");  
-
-# Done.
-# #############################################################################
-$sb->wipe_clean($master_dbh);
-ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox");
-done_testing;

=== modified file 't/pt-online-schema-change/bugs.t'
--- t/pt-online-schema-change/bugs.t	2013-06-26 20:28:27 +0000
+++ t/pt-online-schema-change/bugs.t	2014-08-05 15:51:35 +0000
@@ -124,7 +124,11 @@
 {
    my $o = new OptionParser(file => "$trunk/bin/pt-table-checksum");
    $o->get_specs();
+
+   $o->set('check-plan', 1);  #  check-plan is true by default
+
    no warnings;
+
    local *pt_online_schema_change::explain_statement = sub {
       return { key => 'some_key' }
    };
@@ -369,6 +373,66 @@
    "Bug 1188264: warning about expected MySQL error 1265"
 );
 
+
+# #############################################################################
+# Issue 1315130
+# Failed to detect child tables in other schema, and falsely identified
+# child tables in own schema
+# #############################################################################
+
+$sb->load_file('master', "$sample/bug-1315130_cleanup.sql");
+$sb->load_file('master', "$sample/bug-1315130.sql");
+
+$output = output(
+   sub { pt_online_schema_change::main(@args, "$master_dsn,D=bug_1315130_a,t=parent_table",
+         '--dry-run', 
+         '--alter', "add column c varchar(16)",
+         '--alter-foreign-keys-method', 'auto'),
+      },
+);
+
+   like(
+         $output,
+         qr/Child tables:\s*`bug_1315130_a`\.`child_table_in_same_schema` \(approx\. 1 rows\)\s*`bug_1315130_b`\.`child_table_in_second_schema` \(approx\. 1 rows\)[^`]*?Will/s,
+         "Correctly identify child tables from other schemas and ignores tables from same schema referencig same named parent in other schema.",
+   );
+# clear databases with their foreign keys
+$sb->load_file('master', "$sample/bug-1315130_cleanup.sql");  
+
+
+# #############################################################################
+# Issue 1340728
+# fails when no index is returned in EXPLAIN,  even though --nocheck-plan is set
+# (happens on HASH indexes)
+# #############################################################################
+
+$sb->load_file('master', "$sample/bug-1340728_cleanup.sql");
+$sb->load_file('master', "$sample/bug-1340728.sql");
+
+# insert a few thousand rows (else test isn't valid)
+my $rows = 5000;
+for (my $i = 0; $i < $rows; $i++) {
+   $master_dbh->do("INSERT INTO bug_1340728.test VALUES (NULL, 'xx')");
+}
+
+
+$output = output(
+   sub { pt_online_schema_change::main(@args, "$master_dsn,D=bug_1340728,t=test",
+         '--execute', 
+         '--alter', "ADD COLUMN c INT",
+         '--nocheck-plan',
+         ),
+      },
+);
+
+   like(
+         $output,
+         qr/Successfully altered/s,
+         "--nocheck-plan ignores plans without index",
+   );
+# clear databases 
+$sb->load_file('master', "$sample/bug-1340728_cleanup.sql");  
+
 # #############################################################################
 # Done.
 # #############################################################################

=== added file 't/pt-online-schema-change/samples/bug-1340728.sql'
--- t/pt-online-schema-change/samples/bug-1340728.sql	1970-01-01 00:00:00 +0000
+++ t/pt-online-schema-change/samples/bug-1340728.sql	2014-08-05 15:51:35 +0000
@@ -0,0 +1,10 @@
+drop database if exists bug_1340728;
+create database bug_1340728;
+use bug_1340728;
+
+CREATE TABLE `test` (
+  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
+  `name` char(2) NOT NULL,
+  PRIMARY KEY (`id`) USING HASH
+) ENGINE=MEMORY DEFAULT CHARSET=latin1;
+

=== added file 't/pt-online-schema-change/samples/bug-1340728_cleanup.sql'
--- t/pt-online-schema-change/samples/bug-1340728_cleanup.sql	1970-01-01 00:00:00 +0000
+++ t/pt-online-schema-change/samples/bug-1340728_cleanup.sql	2014-08-05 15:51:35 +0000
@@ -0,0 +1,2 @@
+drop database if exists bug_1340728;
+