Rename _find_renamed_cols() to find_renamed_cols() and add unit tests in find_renamed_cols.t. Tweak dry-run and error message re --check-alter.

bin/pt-online-schema-change

@@ -7784,6 +7784,7 @@ sub main {
    # or die if the user specified a bad alter statement.
    # #####################################################################
    my %renamed_cols;
+
    if ( my $alter = $o->get('alter') ) {
       print "Altering new table...\n";
       my $sql = "ALTER TABLE $new_tbl->{name} $alter";
@@ -7796,20 +7797,25 @@ sub main {
          die "Error altering new table $new_tbl->{name}: $EVAL_ERROR\n"
       }
       print "Altered $new_tbl->{name} OK.\n";
-      %renamed_cols = _find_renamed_cols($alter, $tp);
-      PTDEBUG && _d("Renamed columns (old => new): ", Dumper(\%renamed_cols));
 
+      # Check for renamed columns.
+      # https://bugs.launchpad.net/percona-toolkit/+bug/1068562
+      %renamed_cols = find_renamed_cols($alter, $tp);
+      PTDEBUG && _d("Renamed columns (old => new): ", Dumper(\%renamed_cols));
       if ( %renamed_cols && $o->get('check-alter') ) {
-         my $msg = "Your --alter appears to be renaming these columns: "
-                 . join ", ", map "$_ to $renamed_cols{$_}", keys %renamed_cols;
+         # sort is just for making output consistent for testing
+         my $msg = "--alter appears to be rename these columns: "
+                 . join(", ", map { "$_ to $renamed_cols{$_}" }
+                              sort keys %renamed_cols);
          if ( $o->get('dry-run') ) {
             print $msg . "\n"
          }
          else {
-            die  $msg
-               . ". While the tool should handle this, we recommend testing it "
-               . "first with --dry-run before you run the tool with "
-               . "--no-check-alter to disable this error."
+            die $msg
+               . ".  The tool should handle this correctly, but you should "
+               . " test it first because if it fails the renamed columns' "
+               . " data will be lost!  Specify --no-check-alter to disable "
+               . " this check and perform the --alter.\n";
          }
       }
    }
@@ -7871,7 +7877,10 @@ sub main {
       create_triggers(
          orig_tbl     => $orig_tbl,
          new_tbl      => $new_tbl,
-         columns      => { old_columns => \@old_cols, new_columns => \@new_cols },
+         columns      => {
+            old_columns => \@old_cols,
+            new_columns => \@new_cols,
+         },
          Cxn          => $cxn,
          Quoter       => $q,
          OptionParser => $o,
@@ -8359,7 +8368,7 @@ sub main {
 # Subroutines.
 # ############################################################################
 
-sub _find_renamed_cols {
+sub find_renamed_cols {
    my ($alter, $tp) = @_;
 
    my $unquoted_ident = qr/

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

@@ -0,0 +1,115 @@
+#!/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;
+
+require "$trunk/bin/pt-online-schema-change";
+
+my $q  = Quoter->new;
+my $tp = TableParser->new(Quoter => $q);
+
+sub test_func {
+   my ($alter, $renamed_cols) = @_;
+   die "No alter arg" unless $alter;
+   die "No renamed_cols arg" unless $renamed_cols;
+
+   my %got_renamed_cols = eval {
+      pt_online_schema_change::find_renamed_cols($alter, $tp);
+   };
+   if ( $EVAL_ERROR ) {
+      is_deeply(
+         undef,
+         $renamed_cols,
+         $alter,
+      ) or diag($EVAL_ERROR);
+   }
+   else {
+      is_deeply(
+         \%got_renamed_cols,
+         $renamed_cols,
+         $alter,
+      ) or diag(Dumper(\%got_renamed_cols));
+   }
+}
+
+# #############################################################################
+# Single column alters
+# #############################################################################
+
+test_func(
+   "change old_column_name new_column_name varchar(255) NULL",
+   {
+      old_column_name => 'new_column_name',
+   },
+);
+
+# Case-sensitive?
+test_func(
+   "CHANGE old_column_name new_column_name VARCHAR(255) NULL",
+   {
+      old_column_name => 'new_column_name',
+   },
+);
+
+# Space-sensitive?
+test_func(
+   "CHANGE   a       z     VARCHAR(255)  NULL",
+   {
+      a => 'z',
+   },
+);
+
+# Backtick-sensitive?
+test_func(
+   "CHANGE `a` `z` VARCHAR(255)  NULL",
+   {
+      a => 'z',
+   },
+);
+
+# Extended ascii?
+test_func(
+   "CHANGE `café` `tête-à-tête` INT",
+   {
+      'café' => 'tête-à-tête',
+   },
+);
+
+# #############################################################################
+# Two column alters
+# #############################################################################
+
+test_func(
+   "CHANGE a z VARCHAR(255)  NULL, CHANGE foo bar INT",
+   {
+      a   => 'z',
+      foo => 'bar',
+   },
+);
+
+# #############################################################################
+# Fake alters
+# #############################################################################
+
+# Not really renamed.
+test_func(
+   "CHANGE foo foo FLOAT",
+   {
+   },
+);
+
+# #############################################################################
+# Done.
+# #############################################################################
+done_testing;