Test and fix --no-swap-tables and --no-drop-new-table.

bin/pt-online-schema-change

@@ -5006,6 +5006,10 @@ sub main {
       {
          $o->save_error("Invalid --alter-foreign-keys-method value: $alter_fk_method");
       }
+
+      if ( $alter_fk_method eq 'drop_swap' && !$o->get('drop-new-table') ) {
+         $o->save_error("--alter-foreign-keys-method=drop_swap does not work with --no-drop-new-table.");
+      }
    }
 
    $o->usage_or_errors(); 
@@ -5998,6 +6002,9 @@ sub main {
       if ( $o->get('dry-run') ) {
          print "Not dropping old table because this is a dry run.\n";
       }
+      elsif ( !$old_tbl ) {
+         print "Not dropping old table because --no-swap-tables was specified.\n";
+      }
       else {
          print "Dropping old table...\n";
 
@@ -6993,6 +7000,8 @@ against it will result in an error.  Secondly, if there is an error and the new
 table cannot be renamed into the place of the old one, then it is too late to
 abort, because the old table is gone permanently.
 
+This method forces C<--no-swap-tables> and C<--no-drop-old-table>.
+
 =item none
 
 This method is like C<drop_swap> without the "swap".  Any foreign keys that
@@ -7166,6 +7175,13 @@ default: yes
 
 Drop the new table if copying the original table fails.
 
+Specifying C<--no-drop-new-table> and C<--no-swap-tables> leaves the new,
+altered copy of the table without modifying the original table.  The new
+table name is like C<_TBL_new> where C<TBL> is the table name.
+
+L<--no-drop-new-table> does not work with
+C<alter-foreign-keys-method drop_swap>.
+
 =item --[no]drop-old-table
 
 default: yes
@@ -7175,6 +7191,8 @@ successfully renamed to let the new table take its place, and if there are no
 errors, the tool drops the original table by default. If there are any errors,
 the tool leaves the original table in place.
 
+If C<--no-swap-tables> is specified, then there is no old table to drop.
+
 =item --dry-run
 
 Create and alter the new table, but do not create triggers, copy data, or

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

@@ -10,6 +10,7 @@ use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
 use Test::More;
+use Time::HiRes qw(sleep);
 
 use PerconaTest;
 use Sandbox;
@@ -32,7 +33,7 @@ elsif ( !$slave_dbh ) {
    plan skip_all => 'Cannot connect to sandbox slave';
 }
 else {
-   plan tests => 105;
+   plan tests => 118;
 }
 
 my $q      = new Quoter();
@@ -95,6 +96,7 @@ sub test_alter_table {
 
    if ( my $file = $args{file} ) {
       $sb->load_file('master', "$sample/$file");
+      sleep 0.25;
       if ( my $wait = $args{wait} ) {
          PerconaTest::wait_for_table($slave_dbh, @$wait);
       }
@@ -136,6 +138,15 @@ sub test_alter_table {
       }
    }
 
+   # If --no-drop-new-table is given, then the new, altered table
+   # should still exist, but not yet, so add it to the list so
+   # is_deeply() against $new_tbls passes.  This only works for
+   # single-table tests.
+   my $new_tbl = "_${tbl}_new";
+   if ( grep { $_ eq '--no-drop-new-table' } @$cmds ) {
+      unshift @$orig_tbls, [$new_tbl];
+   }
+
    # TODO: output() is capturing if this call dies, so if a test
    # causes it to die, the tests just stop without saying why, i.e.
    # without re-throwing the error.
@@ -174,6 +185,16 @@ sub test_alter_table {
       "$name rows"
    ) or $fail = 1;
 
+   if ( grep { $_ eq '--no-drop-new-table' } @$cmds ) {
+      $new_rows = $master_dbh->selectall_arrayref(
+         "SELECT * FROM `$db`.`$new_tbl` ORDER BY `$pk_col`");
+      is_deeply(
+         $new_rows,
+         $orig_rows,
+         "$name new table rows"
+      ) or $fail = 1;
+   }
+
    my $new_max_id = $master_dbh->selectall_arrayref(
       "SELECT MAX(`$pk_col`) FROM `$db`.`$tbl`");
    is(
@@ -613,6 +634,31 @@ test_table(
    name => "Space column",
 );
 
+# #############################################################################
+# --[no]swap-tables
+# #############################################################################
+
+test_alter_table(
+   name       => "--no-swap-tables",
+   table      => "pt_osc.t",
+   file       => "basic_no_fks.sql",
+   max_id     => 20,
+   test_type  => "new_engine",  # Engine doesn't actually change
+   new_engine => "MyISAM",      # because the tables aren't swapped
+   cmds       => [qw(--execute --alter ENGINE=InnoDB --no-swap-tables)],
+);
+
+test_alter_table(
+   name       => "--no-swap-tables --no-drop-new-table",
+   table      => "pt_osc.t",
+   file       => "basic_no_fks.sql",
+   max_id     => 20,
+   test_type  => "new_engine",  # Engine doesn't actually change
+   new_engine => "MyISAM",      # because the tables aren't swapped
+   cmds       => [qw(--execute --alter ENGINE=InnoDB --no-swap-tables),
+                  qw(--no-drop-new-table)],
+);
+
 # #############################################################################
 # Done.
 # #############################################################################

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

@@ -9,7 +9,7 @@ BEGIN {
 use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
-use Test::More tests => 5;
+use Test::More tests => 6;
 
 use PerconaTest;
 
@@ -51,6 +51,13 @@ like(
    "--execute FALSE by default"
 );
 
+$output = `$cmd h=127.1,P=12345,u=msandbox,p=msandbox --alter-foreign-keys-method drop_swap --no-drop-new-table`;
+like(
+   $output,
+   qr/--alter-foreign-keys-method=drop_swap does not work with --no-drop-new-table/,
+   "Cannot --alter-foreign-keys-method=drop_swap with --no-drop-new-table"
+);
+
 # #############################################################################
 # Done.
 # #############################################################################