Merge pull request #944 from percona/PT-2407_pt-online-schema-change_exit_stauts_return_code_is_0_even_if_it_does_NOT_succeed

PT-2407 - pt-online-schema-change exit status(return code) is 0 even …

bin/pt-online-schema-change

@@ -9607,11 +9607,16 @@ sub main {
          #                   ''
          #   doesn't match '(?-xism:Failed to find a unique new table name)'
 
-         # (*) Frank:  commented them out because it caused infinite loop
-         # and the mentioned test error doesn't arise
-
          my $original_error = $EVAL_ERROR;
-         my $original_error_code = $?;
+         my $original_error_code;
+         if ( $? ) {
+            $original_error_code = $?;
+         } 
+         else {
+            $original_error_code = $!;
+         }
+
+         $SIG{__DIE__} = 'DEFAULT';
 
          foreach my $task ( reverse @cleanup_tasks ) {
             eval {
@@ -9921,10 +9926,12 @@ sub main {
          $cxn->dbh()->do($sql);
       };
       if ( $EVAL_ERROR ) {
-        if ( $plugin && $plugin->can('before_die') ) {
-          $plugin->before_die(exit_status => $EVAL_ERROR);
-        }
+         if ( $plugin && $plugin->can('before_die') ) {
+            $plugin->before_die(exit_status => $EVAL_ERROR);
+         }
          # this is trapped by a signal handler. Don't replace it with _die
+         # we need to override $SIG{__DIE__} to return correct error code
+         $SIG{__DIE__} = sub { print(STDERR "$_[0]"); exit ERROR_ALTERING_TABLE;  };
          die "Error altering new table $new_tbl->{name}: $EVAL_ERROR\n";
       }
       print "Altered $new_tbl->{name} OK.\n";

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

@@ -0,0 +1,71 @@
+#!/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 PerconaTest;
+use Sandbox;
+require "$trunk/bin/pt-online-schema-change";
+require VersionParser;
+
+use Data::Dumper;
+
+my $dp         = new DSNParser(opts=>$dsn_opts);
+my $sb         = new Sandbox(basedir => '/tmp', DSNParser => $dp);
+my $source_dbh = $sb->get_dbh_for('source');
+my $replica_dbh  = $sb->get_dbh_for('replica1');
+
+if ( !$source_dbh ) {
+   plan skip_all => 'Cannot connect to sandbox source';
+}
+elsif ( !$replica_dbh ) {
+   plan skip_all => 'Cannot connect to sandbox replica';
+}
+
+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";
+
+$sb->load_file('source', "$sample/pt-2407.sql");
+
+($output, $exit) = full_output(
+   sub { pt_online_schema_change::main(@args, "$dsn,D=pt_2407,t=t1",
+         '--alter', 'alter table t1 ADD COLUMN payout_group_id VARCHAR(255) DEFAULT NULL, ALGORITHM=INSTANT;', '--execute') }
+);
+
+is(
+   $exit,
+   11,
+   'Return code non-zero for failed operation'
+) or diag($exit);
+
+like(
+   $output,
+   qr/You have an error in your SQL syntax/,
+   'Job failed due to SQL syntax error'
+) or diag($output);
+
+like(
+   $output,
+   qr/Error altering new table/,
+   'Error altering new table message printed'
+) or diag($output);
+
+# #############################################################################
+# Done.
+# #############################################################################
+
+$sb->wipe_clean($source_dbh);
+ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox");
+#
+done_testing;

t/pt-online-schema-change/samples/pt-2407.sql

@@ -0,0 +1,12 @@
+CREATE DATABASE pt_2407;
+
+USE pt_2407;
+
+CREATE TABLE t1 (
+    c1 int NOT NULL,
+    c2 varchar(100) NOT NULL,
+    PRIMARY KEY (c1),
+    KEY idx (c2)
+) ENGINE=InnoDB;
+
+INSERT INTO t1 VALUES(1,1),(2,2),(3,3),(4,4),(5,5);