PT-169 Fixed drop_swap error handling

bin/pt-online-schema-change

@@ -8788,6 +8788,7 @@ sub main {
    my $flow_ctl_pr;    # Progress for FlowControlWaiter
    my $sys_load;       # MySQLStatusWaiter object
    my $sys_load_pr;    # Progress for MySQLStatusWaiter object
+   my $process_error;  # Used if drop_swap fails
 
    if ( $o->get('execute') ) {
       # #####################################################################
@@ -9150,7 +9151,7 @@ sub main {
          }
          # No child tables and --alter-fk-method wasn't specified,
          # so nothing to do.
-      }
+      } 
       else {
          print "Child tables:\n";
          foreach my $child_table ( @$child_tables ) {
@@ -9414,6 +9415,9 @@ sub main {
             . "cause MySQL error 1146 (42S02): \"Table $new_tbl->{name} "
             . " doesn't exist\".\n";
       }
+      elsif ($process_error) {
+          print "Not dropping new table because FKs processing has failed.\n";
+      }
       else {
          print ts("Dropping new table...\n");
          print $sql, "\n" if $o->get('print');
@@ -10208,8 +10212,9 @@ sub main {
          }
       };
       if ( $EVAL_ERROR ) {
-         # TODO: improve error message and handling.
+          #$oktorun = 0;
-         _die("Error updating foreign key constraints: $EVAL_ERROR", ERROR_UPDATING_FKS);
+          $process_error = "Error updating foreign key constraints: $EVAL_ERROR";
+         _die($process_error, ERROR_UPDATING_FKS);
       }
 
       # --plugin hook
@@ -10273,7 +10278,8 @@ sub main {
    # ########################################################################
    # Done.
    # ########################################################################
-   $orig_tbl->{success} = 1;  # flag for cleanup tasks
+   
+   $orig_tbl->{success} = 1;  # flag for cleanup tasks   
    $cleanup = undef;          # exec cleanup tasks
 
    # --plugin hook

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

@@ -0,0 +1,76 @@
+#!/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 threads;
+
+use English qw(-no_match_vars);
+use Test::More;
+
+use Data::Dumper;
+use PerconaTest;
+use Sandbox;
+use SqlModes;
+use File::Temp qw/ tempdir /;
+
+plan tests => 3;
+
+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 $master_dsn = 'h=127.1,P=12345,u=msandbox,p=msandbox';
+
+if ( !$master_dbh ) {
+   plan skip_all => 'Cannot connect to sandbox master';
+}
+
+# The sandbox servers run with lock_wait_timeout=3 and it's not dynamic
+# so we need to specify --set-vars innodb_lock_wait_timeout=3 else the
+# tool will die.
+my @args       = (qw(--set-vars innodb_lock_wait_timeout=3));
+my $output;
+my $exit_status;
+my $sample  = "t/pt-online-schema-change/samples/";
+my $ERROR_UPDATING_FKS = 15; # from pt-online-schema-change line 8453
+
+$sb->load_file('master', "$sample/pt-169.sql");
+
+($output, $exit_status) = full_output(
+   sub { pt_online_schema_change::main(@args, "$master_dsn,D=test,t=users",
+         '--execute', '--alter', 'CHANGE COLUMN id id BIGINT UNSIGNED NOT NULL FIRST', 
+         '--set-vars', 'foreign_key_checks=0',
+         '--alter-foreign-keys-method', 'drop_swap',  '--no-check-alter')
+      },
+);
+
+is(
+      $exit_status,
+      $ERROR_UPDATING_FKS,
+      "--alter rename columns with uppercase names -> exit status 0",
+);
+
+# Since drop_swap has failed, the clueanup process should be skipped and the new table
+# shouldn't be deleted
+my $row = $master_dbh->selectrow_hashref("select count(*) AS how_many from test._users_new");
+is (
+    $row->{how_many},
+    1,
+    "Correct number of rows",
+);
+
+$master_dbh->do("DROP DATABASE IF EXISTS test");
+
+# #############################################################################
+# 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/pt-169.sql

@@ -0,0 +1,40 @@
+DROP DATABASE IF EXISTS `test`;
+CREATE DATABASE test;
+USE test;
+
+CREATE TABLE `users` (
+  `id` int(10) unsigned NOT NULL,
+  `username` varchar(255) NOT NULL,
+  `full_name` varchar(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_520_ci NOT NULL,
+  `is_verified` tinyint(1) NOT NULL DEFAULT '0',
+  `is_private` tinyint(1) NOT NULL DEFAULT '0',
+  `profile_pic_url` varchar(255) DEFAULT NULL,
+  `follower_count` int(11) NOT NULL DEFAULT '0',
+  `following_count` int(11) NOT NULL DEFAULT '0',
+  `media_count` int(11) NOT NULL DEFAULT '0',
+  `biography` varchar(512) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_520_ci DEFAULT NULL,
+  `user_active` tinyint(1) NOT NULL DEFAULT '1',
+  PRIMARY KEY (`id`),
+  KEY `username` (`username`),
+  KEY `follower_count` (`follower_count`),
+  KEY `following_count` (`following_count`)
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+
+CREATE TABLE `user_comments` (
+  `id_user_comments` int(11) NOT NULL AUTO_INCREMENT,
+  `msg` varchar(255) DEFAULT NULL,
+  `user_id` int(10) unsigned DEFAULT NULL,
+  PRIMARY KEY (`id_user_comments`),
+  KEY `fk1_idx` (`user_id`),
+  CONSTRAINT `fk1` FOREIGN KEY (`user_id`) REFERENCES `users` (`id`) ON DELETE NO ACTION ON UPDATE NO ACTION
+) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=latin1;
+
+
+INSERT INTO `test`.`users` (`id`, `username`, `full_name`, `is_verified`, `is_private`, `profile_pic_url`,
+`follower_count`, `following_count`, `media_count`, `biography`, `user_active`)
+VALUES
+(1, "zappb", "zapp brannigan", 1, 1, "https://pbs.twimg.com/profile_images/447660347273408512/NdZEGKvr.jpeg", 0, 0, 0, "", 1);
+ 
+INSERT INTO `test`.`user_comments` (`id_user_comments`, `msg`, `user_id`)
+VALUES
+(1, "I am the man with no name. Zapp Brannigan, at your service.", 1);

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

@@ -23,7 +23,9 @@ if ($ENV{PERCONA_SLOW_BOX}) {
     plan skip_all => 'This test needs a fast machine';
 } else {
     plan tests => 6;
-}
+    #plan skip_all => 'This test is taking too much time even in fast machines';
+}                                  
+
 our $delay = 30;
 
 my $tmp_file = File::Temp->new();
@@ -39,10 +41,6 @@ my $slave_dbh = $sb->get_dbh_for('slave1');
 my $master_dsn = 'h=127.0.0.1,P=12345,u=msandbox,p=msandbox';
 my $slave_dsn = 'h=127.0.0.1,P=12346,u=msandbox,p=msandbox';
 
-if ( !$master_dbh ) {
-   plan skip_all => 'Cannot connect to sandbox master';
-}
-
 sub reset_query_cache {
     my @dbhs = @_;
     return if ($sandbox_version >= '8.0');
@@ -64,7 +62,7 @@ $sb->load_file('master', "t/pt-online-schema-change/samples/slave_lag.sql");
 
 my $num_rows = 5000;
 diag("Loading $num_rows into the table. This might take some time.");
-diag(`util/mysql_random_data_load --host=127.0.0.1 --port=12345 --user=msandbox --password=msandbox test pt178 --bulk-size=1 --max-threads=1 $num_rows`);
+diag(`util/mysql_random_data_load --host=127.0.0.1 --port=12345 --user=msandbox --password=msandbox test pt178 $num_rows`);
 
 diag("Setting slave delay to $delay seconds");