Merge fix-osc-index-bug-994002.

bin/pt-online-schema-change

@@ -2815,7 +2815,7 @@ sub name {
 
 sub DESTROY {
    my ($self) = @_;
-   if ( $self->{dbh} ) {
+   if ( $self->{dbh} && ref($self->{dbh}) ) {
       PTDEBUG && _d('Disconnecting dbh', $self->{dbh}, $self->{name});
       $self->{dbh}->disconnect();
    }
@@ -4126,8 +4126,8 @@ sub next {
    while ( $self->{have_rows} || $self->_next_boundaries() ) {
       if ( !$self->{have_rows} ) {
          $self->{nibbleno}++;
-         PTDEBUG && _d($self->{nibble_sth}->{Statement}, 'params:',
-            join(', ', (@{$self->{lower}}, @{$self->{upper}})));
+         PTDEBUG && _d('Nibble:', $self->{nibble_sth}->{Statement}, 'params:',
+            join(', ', (@{$self->{lower} || []}, @{$self->{upper} || []})));
          if ( my $callback = $self->{callbacks}->{exec_nibble} ) {
             $self->{have_rows} = $callback->(%callback_args);
          }
@@ -4257,14 +4257,21 @@ sub can_nibble {
    }
    my ($cxn, $tbl, $chunk_size, $o) = @args{@required_args};
 
+   my $where = $o->has('where') ? $o->get('where') : '';
+
    my ($row_est, $mysql_index) = get_row_estimate(
       Cxn   => $cxn,
       tbl   => $tbl,
-      where => $o->has('where') ? $o->get('where') : '',
+      where => $where,
    );
 
+   if ( !$where ) {
+      $mysql_index = undef;
+   }
+
+   my $chunk_size_limit = $o->get('chunk-size-limit') || 1;
    my $one_nibble = !defined $args{one_nibble} || $args{one_nibble}
-                  ? $row_est <= $chunk_size * $o->get('chunk-size-limit')
+                  ? $row_est <= $chunk_size * $chunk_size_limit
                   : 0;
    PTDEBUG && _d('One nibble:', $one_nibble ? 'yes' : 'no');
 
@@ -4392,7 +4399,11 @@ sub get_row_estimate {
    PTDEBUG && _d($sql);
    my $expl = $cxn->dbh()->selectrow_hashref($sql);
    PTDEBUG && _d(Dumper($expl));
-   return ($expl->{rows} || 0), $expl->{key};
+   my $mysql_index = $expl->{key} || '';
+   if ( $mysql_index ne 'PRIMARY' ) {
+      $mysql_index = lc($mysql_index);
+   }
+   return ($expl->{rows} || 0), $mysql_index;
 }
 
 sub _prepare_sths {
@@ -4445,12 +4456,10 @@ sub _get_bounds {
 
    if ( !$self->{next_lower} ) {
       PTDEBUG && _d('At end of table, or no more boundaries to resume');
+      $self->{no_more_boundaries} = 1;
 
       $self->{last_upper} = $dbh->selectrow_arrayref($self->{last_ub_sql});
       PTDEBUG && _d('Last upper boundary:', Dumper($self->{last_upper}));
-      $self->{no_more_boundaries} = 1;
-
-      $self->{no_more_boundaries} = 1;
    }
 
    return;
@@ -4470,12 +4479,15 @@ sub _next_boundaries {
       return 1; # continue nibbling
    }
 
+
+
    if ( $self->identical_boundaries($self->{lower}, $self->{next_lower}) ) {
       PTDEBUG && _d('Infinite loop detected');
       my $tbl     = $self->{tbl};
       my $index   = $tbl->{tbl_struct}->{keys}->{$self->{index}};
       my $n_cols  = scalar @{$index->{cols}};
       my $chunkno = $self->{nibbleno};
+
       die "Possible infinite loop detected!  "
          . "The lower boundary for chunk $chunkno is "
          . "<" . join(', ', @{$self->{lower}}) . "> and the lower "
@@ -4502,6 +4514,7 @@ sub _next_boundaries {
       }
    }
 
+
    PTDEBUG && _d($self->{ub_sth}->{Statement}, 'params:',
       join(', ', @{$self->{lower}}), $self->{limit});
    $self->{ub_sth}->execute(@{$self->{lower}}, $self->{limit});
@@ -4860,8 +4873,13 @@ sub new {
 sub DESTROY {
    my ($self) = @_;
    my $task = $self->{task};
-   PTDEBUG && _d('Calling cleanup task', $task);
-   $task->();
+   if ( ref $task ) {
+      PTDEBUG && _d('Calling cleanup task', $task);
+      $task->();
+   }
+   else {
+      warn "Lost cleanup task";
+   }
    return;
 }
 

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

@@ -0,0 +1,73 @@
+#!/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 Data::Dumper;
+use PerconaTest;
+use Sandbox;
+
+require "$trunk/bin/pt-online-schema-change";
+
+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 slave1';
+}
+else {
+   plan tests => 2;
+}
+
+# The sandbox servers run with lock_wait_timeout=3 and it's not dynamic
+# so we need to specify --lock-wait-timeout=3 else the tool will die.
+my $master_dsn = 'h=127.1,P=12345,u=msandbox,p=msandbox';
+my @args       = (qw(--lock-wait-timeout 3));
+my $output;
+my $exit_status;
+my $sample  = "t/pt-online-schema-change/samples/";
+
+# ############################################################################
+# https://bugs.launchpad.net/percona-toolkit/+bug/994002
+# pt-online-schema-change 2.1.1 doesn't choose the PRIMARY KEY
+# ############################################################################
+$sb->load_file('master', "$sample/pk-bug-994002.sql");
+
+$output = output(
+   sub { $exit_status = pt_online_schema_change::main(@args,
+      "$master_dsn,D=test,t=t",
+      "--alter", "add column (foo int)",
+      qw(--chunk-size 2 --dry-run --print)) },
+);
+
+# Must chunk the table to detect the next test correctly.
+like(
+   $output,
+   qr/next chunk boundary/,
+   "Bug 994002: chunks the table"
+);
+
+unlike(
+   $output,
+   qr/FORCE INDEX\(`guest_language`\)/,
+   "Bug 994002: doesn't choose non-PK"
+);
+
+# #############################################################################
+# Done.
+# #############################################################################
+$sb->wipe_clean($master_dbh);
+exit;

t/pt-online-schema-change/samples/pk-bug-994002.sql

@@ -0,0 +1,29 @@
+drop database if exists test;
+create database test;
+use test;
+
+CREATE TABLE t (
+  `ufi` int(11) NOT NULL,
+  `guest_language` char(2) NOT NULL,
+  `guest_country` char(2) NOT NULL,
+  `score` int(10) unsigned NOT NULL,
+  PRIMARY KEY (`ufi`,`guest_language`,`guest_country`),
+  KEY `guest_language` (`guest_language`,`guest_country`,`score`)
+) ENGINE=InnoDB;
+
+INSERT INTO t VALUES
+  (1, 'en', 'en', 1),
+  (2, 'fr', 'fr', 1),
+  (3, 'es', 'es', 1),
+  (4, 'ru', 'ru', 1),
+  (5, 'sl', 'sl', 1),
+  (6, 'ch', 'ch', 1),
+  (7, 'en', 'en', 1),
+  (8, 'fr', 'fr', 1),
+  (9, 'es', 'es', 1),
+  (10,'ru', 'ru', 1),
+  (11,'sl', 'sl', 1),
+  (12,'aa', 'ch', 1),
+  (10,'ab', 'ru', 1),
+  (11,'ac', 'sl', 1),
+  (12,'ad', 'ch', 1);