Merge pk-del-bug-1103672-encore.

bin/pt-online-schema-change

@@ -8370,19 +8370,74 @@ sub main {
    # Find a pk or unique index to use for the delete trigger.  can_nibble()
    # above returns an index, but NibbleIterator will use non-unique indexes,
    # so we have to do this again here.
-   my $indexes = $new_tbl->{tbl_struct}->{keys}; # brevity
+   {
-   foreach my $index ( $tp->sort_indexes($new_tbl->{tbl_struct}) ) {
+      my $indexes = $new_tbl->{tbl_struct}->{keys}; # brevity
-      if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) {
+      foreach my $index ( $tp->sort_indexes($new_tbl->{tbl_struct}) ) {
-         PTDEBUG && _d('Delete trigger index:', Dumper($index));
+         if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) {
-         $new_tbl->{del_index} = $index;
+            PTDEBUG && _d('Delete trigger new index:', Dumper($index));
-         last;
+            $new_tbl->{del_index} = $index;
+            last;
+         }
       }
+      PTDEBUG && _d('New table delete index:', $new_tbl->{del_index});
    }
+
+   {
+      my $indexes = $orig_tbl->{tbl_struct}->{keys}; # brevity
+      foreach my $index ( $tp->sort_indexes($orig_tbl->{tbl_struct}) ) {
+         if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) {
+            PTDEBUG && _d('Delete trigger orig index:', Dumper($index));
+            $orig_tbl->{del_index} = $index;
+            last;
+         }
+      }
+      PTDEBUG && _d('Orig table delete index:', $orig_tbl->{del_index});
+   }
+
    if ( !$new_tbl->{del_index} ) {
       die "The new table $new_tbl->{name} does not have a PRIMARY KEY "
         . "or a unique index which is required for the DELETE trigger.\n";
    }
 
+   # Determine whether to use the new or orig table delete index.
+   # The new table del index is preferred due to
+   # https://bugs.launchpad.net/percona-toolkit/+bug/1062324
+   # In short, if the chosen del index is re-created with new columns,
+   # its original columns may be dropped, so just use its new columns.
+   # But, due to https://bugs.launchpad.net/percona-toolkit/+bug/1103672,
+   # the chosen del index on the new table may reference columns which
+   # do not/no longer exist in the orig table, so we check for this
+   # and, if it's the case, we fall back to using the del index from
+   # the orig table.
+   my $del_tbl = $new_tbl; # preferred
+   my $new_del_index_cols  # brevity
+      = $new_tbl->{tbl_struct}->{keys}->{ $new_tbl->{del_index} }->{cols};
+   foreach my $new_del_index_col ( @$new_del_index_cols ) {
+      if ( !exists $orig_cols->{$new_del_index_col} ) {
+         if ( !$orig_tbl->{del_index} ) {
+            die "The new table index $new_tbl->{del_index} would be used "
+               . "for the DELETE trigger, but it uses column "
+               . "$new_del_index_col which does not exist in the original "
+               . "table and the original table does not have a PRIMARY KEY "
+               . "or a unique index to use for the DELETE trigger.\n";
+         }
+         print "Using original table index $orig_tbl->{del_index} for the "
+            . "DELETE trigger instead of new table index $new_tbl->{del_index} "
+            . "because the new table index uses column $new_del_index_col "
+            . "which does not exist in the original table.\n";
+         $del_tbl = $orig_tbl;
+         last;
+      }
+   }
+
+   {
+      my $del_cols
+         = $del_tbl->{tbl_struct}->{keys}->{ $del_tbl->{del_index} }->{cols};
+      PTDEBUG && _d('Index for delete trigger: table', $del_tbl->{name},
+         'index', $del_tbl->{del_index},
+         'columns', @$del_cols);
+   }
+
    # ########################################################################
    # Step 3: Create the triggers to capture changes on the original table and
    # apply them to the new table.
@@ -8412,6 +8467,7 @@ sub main {
       create_triggers(
          orig_tbl     => $orig_tbl,
          new_tbl      => $new_tbl,
+         del_tbl      => $del_tbl,
          columns      => \@common_cols,
          Cxn          => $cxn,
          Quoter       => $q,
@@ -8930,6 +8986,28 @@ sub check_alter {
 
    my $ok = 1;
 
+   # ########################################################################
+   # Check for DROP PRIMARY KEY.
+   # ########################################################################
+   if ( $alter =~ m/DROP\s+PRIMARY\s+KEY/i ) {
+      my $msg = "--alter contains 'DROP PRIMARY KEY'.  Dropping and "
+              . "altering the primary key can be dangerous, "
+              . "especially if the original table does not have other "
+              . "unique indexes.\n";
+      if ( $dry_run ) {
+         print $msg;
+      }
+      else {
+         $ok = 0;
+         warn $msg
+            . "The tool should handle this correctly, but you should "
+            . "test it first and carefully examine the triggers which "
+            . "rely on the PRIMARY KEY or a unique index.  Specify "
+            . "--no-check-alter to disable this check and perform the "
+            . "--alter.\n";
+      }
+   }
+
    # ########################################################################
    # Check for renamed columns.
    # https://bugs.launchpad.net/percona-toolkit/+bug/1068562
@@ -8976,6 +9054,7 @@ sub check_alter {
    }
 
    if ( !$ok ) {
+      # check_alter.t relies on this output.
       die "--check-alter failed.\n";
    }
 
@@ -9536,11 +9615,11 @@ sub drop_swap {
 
 sub create_triggers {
    my ( %args ) = @_;
-   my @required_args = qw(orig_tbl new_tbl columns Cxn Quoter OptionParser);
+   my @required_args = qw(orig_tbl new_tbl del_tbl columns Cxn Quoter OptionParser);
    foreach my $arg ( @required_args ) {
       die "I need a $arg argument" unless $args{$arg};
    }
-   my ($orig_tbl, $new_tbl, $cols, $cxn, $q, $o) = @args{@required_args};
+   my ($orig_tbl, $new_tbl, $del_tbl, $cols, $cxn, $q, $o) = @args{@required_args};
 
    # This sub works for --dry-run and --execute.  With --dry-run it's
    # only interesting if --print is specified, too; then the user can
@@ -9569,8 +9648,8 @@ sub create_triggers {
    # unique indexes can be nullable.  Cols are from the new table and
    # they may have been renamed
    my %old_col_for    = map { $_->{new} => $_->{old} } @$cols;
-   my $tbl_struct     = $new_tbl->{tbl_struct};
+   my $tbl_struct     = $del_tbl->{tbl_struct};
-   my $del_index      = $new_tbl->{del_index};
+   my $del_index      = $del_tbl->{del_index};
    my $del_index_cols = join(" AND ", map {
       my $new_col  = $_;
       my $old_col  = $old_col_for{$new_col} || $new_col;
@@ -10226,9 +10305,19 @@ In previous versions of the tool, renaming a column with
 C<CHANGE COLUMN name new_name> would lead to that column's data being lost.
 The tool now parses the alter statement and tries to catch these cases, so
 the renamed columns should have the same data as the originals. However, the
-code that does this is not a full-blown SQL parser, so we recommend that users
+code that does this is not a full-blown SQL parser, so you should first
-run the tool with L<"--dry-run"> and check if it's detecting the renames
+run the tool with L<"--dry-run"> and L<"--print"> and verify that it detects
-correctly.
+the renamed columns correctly.
+
+=item DROP PRIMARY KEY
+
+If L<"--alter"> contain C<DROP PRIMARY KEY> (case- and space-insensitive),
+a warning is printed and the tool exits unless L<"--dry-run"> is specified.
+Altering the primary key can be dangerous, but the tool can handle it.
+The tool's triggers, particularly the DELETE trigger, are most affected by
+altering the primary key because the tool prefers to use the primary key
+for its triggers.  You should first run the tool with L<"--dry-run"> and
+L<"--print"> and verify that the triggers are correct.
 
 =back
 

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

@@ -210,7 +210,7 @@ $sb->load_file('master', "$sample/del-trg-bug-1062324.sql");
       sub { pt_online_schema_change::main(@args,
          "$master_dsn,D=test,t=t1",
          "--alter", "drop key 2bpk, drop key c3, drop primary key, drop c1, add primary key (c2, c3(4)), add key (c3(4))",
-         qw(--execute --no-drop-new-table --no-swap-tables)) },
+         qw(--no-check-alter --execute --no-drop-new-table --no-swap-tables)) },
    );
 
    # Since _t1_new no longer has the c1 column, the bug caused this
@@ -233,6 +233,34 @@ $sb->load_file('master', "$sample/del-trg-bug-1062324.sql");
       undef,
       "Delete trigger works after altering PK (bug 1062324)"
    );
+
+   # Another instance of this bug:
+   # https://bugs.launchpad.net/percona-toolkit/+bug/1103672
+   $sb->load_file('master', "$sample/del-trg-bug-1103672.sql");
+
+   ($output, $exit_status) = full_output(
+      sub { pt_online_schema_change::main(@args,
+         "$master_dsn,D=test,t=t1",
+         "--alter", "drop primary key, add column _id int unsigned not null primary key auto_increment FIRST",
+         qw(--no-check-alter --execute --no-drop-new-table --no-swap-tables)) },
+   );
+
+   eval {
+      $master_dbh->do("DELETE FROM test.t1 WHERE id=1");
+   };
+   is(
+      $EVAL_ERROR,
+      "",
+      "No delete trigger error after altering PK (bug 1103672)"
+   ) or diag($output);
+
+   $row = $master_dbh->selectrow_arrayref("SELECT * FROM test._t1_new WHERE id=1");
+   is(
+      $row,
+      undef,
+      "Delete trigger works after altering PK (bug 1103672)"
+   );
+
 }
 
 # #############################################################################

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

@@ -0,0 +1,65 @@
+#!/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';
+}
+
+# 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/";
+
+# #############################################################################
+# DROP PRIMARY KEY
+# #############################################################################
+
+$sb->load_file('master', "$sample/del-trg-bug-1103672.sql");
+
+($output, $exit_status) = full_output(
+   sub { pt_online_schema_change::main(@args,
+      "$master_dsn,D=test,t=t1",
+      "--alter", "drop primary key, add column _id int unsigned not null primary key auto_increment FIRST",
+      qw(--execute)),
+   },
+);
+
+like(
+   $output,
+   qr/--check-alter failed/,
+   "DROP PRIMARY KEY"
+);
+
+# #############################################################################
+# 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/samples/del-trg-bug-1103672.sql

@@ -0,0 +1,20 @@
+drop database if exists test;
+create database test;
+use test;
+
+CREATE TABLE `t1` (
+  `id` int(10) unsigned NOT NULL,
+  `x` char(3) DEFAULT NULL,
+  PRIMARY KEY (`id`)
+) ENGINE=InnoDB;
+
+INSERT INTO t1 VALUES
+  (1, 'a'),
+  (2, 'b'),
+  (3, 'c'),
+  (4, 'd'),
+  (5, 'f'),
+  (6, 'g'),
+  (7, 'h'),
+  (8, 'i'),
+  (9, 'j');