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

bin/pt-online-schema-change

@@ -8908,27 +8908,30 @@ sub main {
          # 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(
+         # Skip if --nocheck-plan   See: https://bugs.launchpad.net/percona-toolkit/+bug/1340728
-            tbl  => $tbl,
+         if ( $o->get('check-plan') ) {
-            sth  => $sth->{explain_upper_boundary},
+            my $expl = explain_statement(
-            vals => [ @{$boundary->{lower}}, $nibble_iter->limit() ],
+               tbl  => $tbl,
-         );
+               sth  => $sth->{explain_upper_boundary},
-         if (lc($expl->{key} || '') ne lc($nibble_iter->nibble_index() || '') and $o->get('check-plan')) {
+               vals => [ @{$boundary->{lower}}, $nibble_iter->limit() ],
-            my $msg
+            );
-               = "Aborting copying table $tbl->{name} at chunk "
+            if ( lc($expl->{key} || '') ne lc($nibble_iter->nibble_index() || '') ) {
-               . ($nibble_iter->nibble_number() + 1)
+               my $msg
-               . " because it is not safe to ascend.  Chunking should "
+                  = "Aborting copying table $tbl->{name} at chunk "
-               . "use the "
+                  . ($nibble_iter->nibble_number() + 1)
-               . ($nibble_iter->nibble_index() || '?')
+                  . " because it is not safe to ascend.  Chunking should "
-               . " index, but MySQL EXPLAIN reports that "
+                  . "use the "
-               . ($expl->{key} ? "the $expl->{key}" : "no")
+                  . ($nibble_iter->nibble_index() || '?')
-               . " index will be used for "
+                  . " index, but MySQL EXPLAIN reports that "
-               . $sth->{upper_boundary}->{Statement}
+                  . ($expl->{key} ? "the $expl->{key}" : "no")
-               . " with values "
+                  . " index will be used for "
-               . join(", ", map { defined $_ ? $_ : "NULL" }
+                  . $sth->{upper_boundary}->{Statement}
-                      (@{$boundary->{lower}}, $nibble_iter->limit()))
+                  . " with values "
-               . "\n";
+                  . join(", ", map { defined $_ ? $_ : "NULL" }
-            die ts($msg);
+                         (@{$boundary->{lower}}, $nibble_iter->limit()))
+                  . "\n";
+               die ts($msg);
+            } 
          }
 
          # Once nibbling begins for a table, control does not return to this
@@ -9532,14 +9535,16 @@ sub nibble_is_safe {
    );
 
    # Ensure that MySQL is using the chunk index if the table is being chunked. 
-   if ( !$nibble_iter->one_nibble()
+   # Skip if --nocheck-plan   See: https://bugs.launchpad.net/percona-toolkit/+bug/1340728
-        && lc($expl->{key} || '') ne lc($nibble_iter->nibble_index() || '') and $o->get('check-plan'))
+      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()
+           && $o->get('check-plan') )
-         . " of $tbl->{db}.$tbl->{tbl} because MySQL chose "
+      {
-         . ($expl->{key} ? "the $expl->{key}" : "no") . " index "
+         die ts("Error copying rows at chunk " . $nibble_iter->nibble_number()
-         . " instead of the " . $nibble_iter->nibble_index() . "index.\n");
+            . " 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
@@ -9566,9 +9571,11 @@ sub nibble_is_safe {
 
    # 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 "

t/pt-online-schema-change/bug-1340728.t

@@ -0,0 +1,86 @@
+#!/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";
+
+
+# #############################################################################
+# 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, $exit) = full_output(
+   sub { pt_online_schema_change::main(@args, "$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.
+# #############################################################################
+$sb->wipe_clean($master_dbh);
+ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox");
+done_testing;

t/pt-online-schema-change/bugs.t

@@ -124,7 +124,11 @@ like(
 {
    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' }
    };

t/pt-online-schema-change/samples/bug-1340728.sql

@@ -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;
+

t/pt-online-schema-change/samples/bug-1340728_cleanup.sql

@@ -0,0 +1,2 @@
+drop database if exists bug_1340728;
+