Merge lp:~percona-toolkit-dev/percona-toolkit/fix-osc-repl-bug-933232 into lp:percona-toolkit/2.0

[Daniel Nichter]

[Daniel Nichter (daniel-nichter)]

review: Approve

=== modified file 'bin/pt-online-schema-change'
--- bin/pt-online-schema-change	2012-02-21 16:06:08 +0000
+++ bin/pt-online-schema-change	2012-03-02 20:34:36 +0000
@@ -4110,6 +4110,13 @@
    # #####################################################################
    # Do the online alter.
    # #####################################################################
+   if ( !$o->get('execute') ) {
+      msg("Exiting without altering $db.$tbl because you did not "
+        . "specify --execute.  Please read the tool's documentation "
+        . "carefully before using this tool.");
+      return $exit_status;
+   }
+
    msg("Starting online schema change");
    eval {
       my $sql = "";
@@ -4218,7 +4225,7 @@
             msg($sql);
             $dbh->do($sql) unless $o->get('print');
             
-            $sql = "DROP TABLE `$db`.`$tbl`";
+            $sql = "DROP TABLE IF EXISTS `$db`.`$tbl`";
             msg($sql);
             $dbh->do($sql) unless $o->get('print');
 
@@ -4241,7 +4248,7 @@
       $capture_sync->cleanup(%plugin_args);
 
       if ( $o->get('rename-tables') &&  $o->get('drop-old-table') ) {
-         $sql = "DROP TABLE `$db`.`$old_tbl`";
+         $sql = "DROP TABLE IF EXISTS `$db`.`$old_tbl`";
          msg($sql);
          $dbh->do($sql) unless $o->get('print');
       }
@@ -4510,12 +4517,12 @@
 are those created by the nature of the tool (e.g. read-only tools vs. read-write
 tools) and those created by bugs.
 
-pt-online-schema-change reads, writes, alters and drops tables.  Although
-it is tested, do not use it in production until you have thoroughly tested it
-in your environment!
+pt-online-schema-change alters tables and data, therefore it is a high-risk
+tool.  Although the tool is tested and is known to work, you should not use
+it in production until you have thoroughly tested it in your environment!
 
-This tool has not been tested with replication; it may break replication.
-See L<"REPLICATION">.
+This tool has not been tested with replication, and it can break replication
+if not used properly.  See L<"REPLICATION">.
 
 At the time of this release there are no known bugs that pose a serious risk.
 
@@ -4604,8 +4611,24 @@
 
 =head1 REPLICATION
 
-In brief: update slaves first if columns are added or removed.  Certain
-ALTER changes like ENGINE may not affect replication.
+If you use pt-online-schema-change to alter a table on a server with slaves,
+be aware that:
+
+=over
+
+=item * The tool is not tested with replication
+
+=item * The tool can break replication if not used properly
+
+=item * Although the tool sets SQL_BIN_LOG=0 by default (unless L<"--bin-log"> is specified), triggers which track changes to the table being altered still write statements to the binary log
+
+=item * Replicaiton will break if you alter a table on a master that does not exist on a slave
+
+=item * Update slaves first if columns are being added or removed
+
+=item * Do not use this tool in production before testing it
+
+=back
 
 =head1 OUTPUT
 
@@ -4737,6 +4760,13 @@
 If altering a table with foreign key constraints, you may need to specify
 this option depending on which L<"--update-foreign-keys-method"> you choose.
 
+=item --execute
+
+Alter the table.  This option must be specified to alter the table, else
+the tool will only check the table and exit.  This helps ensure that the
+user has read the documentation and is aware of the inherent risks of using
+this tool.
+
 =item --[no]foreign-key-checks
 
 default: yes

=== modified file 't/pt-online-schema-change/alter_active_table.t'
--- t/pt-online-schema-change/alter_active_table.t	2011-08-02 21:14:06 +0000
+++ t/pt-online-schema-change/alter_active_table.t	2012-03-02 20:34:36 +0000
@@ -33,7 +33,7 @@
 
 my $output  = "";
 my $cnf     = '/tmp/12345/my.sandbox.cnf';
-my @args    = ('-F', $cnf);
+my @args    = ('-F', $cnf, '--execute');
 my $exit    = 0;
 my $rows;
 

=== modified file 't/pt-online-schema-change/basics.t'
--- t/pt-online-schema-change/basics.t	2012-02-21 21:26:44 +0000
+++ t/pt-online-schema-change/basics.t	2012-03-02 20:34:36 +0000
@@ -19,23 +19,42 @@
 
 my $dp  = new DSNParser(opts=>$dsn_opts);
 my $sb  = new Sandbox(basedir => '/tmp', DSNParser => $dp);
-my $dbh = $sb->get_dbh_for('master');
+my $master_dbh = $sb->get_dbh_for('master');
+my $slave_dbh  = $sb->get_dbh_for('slave1');
 
-if ( !$dbh ) {
+if ( !$master_dbh ) {
    plan skip_all => 'Cannot connect to sandbox master';
 }
+elsif ( !$slave_dbh ) {
+   plan skip_all => 'Cannot connect to sandbox slave';
+}
 else {
-   plan tests => 22;
+   plan tests => 25;
 }
 
 my $output  = "";
 my $cnf     = '/tmp/12345/my.sandbox.cnf';
-my @args    = ('-F', $cnf);
+my @args    = ('-F', $cnf, '--execute');
 my $exit    = 0;
 my $rows;
 
 $sb->load_file('master', "t/pt-online-schema-change/samples/small_table.sql");
-$dbh->do('use mkosc');
+PerconaTest::wait_for_table($slave_dbh, "mkosc.a", "c='r'");
+$master_dbh->do('use mkosc');
+$slave_dbh->do('use mkosc');
+
+# #############################################################################
+# Tool shouldn't run without --execute (bug 933232).
+# #############################################################################
+$output = output(
+   sub { pt_online_schema_change::main('h=127.1,P=12345,u=msandbox,p=msandbox,D=mkosc,t=a') },
+);
+
+like(
+   $output,
+   qr/you did not specify --execute/,
+   "Doesn't run without --execute"
+);
 
 # #############################################################################
 # --check-tables-and-exit
@@ -86,7 +105,7 @@
       'D=mkosc,t=a', qw(--alter ENGINE=InnoDB)) },
 );
 
-$rows = $dbh->selectall_hashref('show table status from mkosc', 'name');
+$rows = $master_dbh->selectall_hashref('show table status from mkosc', 'name');
 is(
    $rows->{a}->{engine},
    'InnoDB',
@@ -99,8 +118,8 @@
    "Kept old table, ENGINE=MyISAM"
 );
 
-my $org_rows = $dbh->selectall_arrayref('select * from mkosc.__old_a order by i');
-my $new_rows = $dbh->selectall_arrayref('select * from mkosc.a order by i');
+my $org_rows = $master_dbh->selectall_arrayref('select * from mkosc.__old_a order by i');
+my $new_rows = $master_dbh->selectall_arrayref('select * from mkosc.a order by i');
 is_deeply(
    $new_rows,
    $org_rows,
@@ -113,10 +132,23 @@
    "Exit status 0"
 );
 
+# Tool should set SQL_LOG_BIN=0 by default, so the table
+# on the slave should not have changed.
+$rows = $slave_dbh->selectall_hashref('show table status from mkosc', 'name');
+is(
+   $rows->{a}->{engine},
+   'MyISAM',
+   "SQL_LOG_BIN=0 by default"
+);
+
 # #############################################################################
 # No --alter and --drop-old-table.
 # #############################################################################
+<<<<<<< TREE
 $dbh->do('drop table if exists mkosc.__old_a');  # from previous run
+=======
+$master_dbh->do('drop table if exists mkosc.__old_a');  # from prev run
+>>>>>>> MERGE-SOURCE
 $sb->load_file('master', "t/pt-online-schema-change/samples/small_table.sql");
 
 output(
@@ -124,7 +156,7 @@
       'D=mkosc,t=a', qw(--drop-old-table)) },
 );
 
-$rows = $dbh->selectall_hashref('show table status from mkosc', 'name');
+$rows = $master_dbh->selectall_hashref('show table status from mkosc', 'name');
 is(
    $rows->{a}->{engine},
    'MyISAM',
@@ -136,7 +168,7 @@
    "--drop-old-table"
 );
 
-$new_rows = $dbh->selectall_arrayref('select * from mkosc.a order by i');
+$new_rows = $master_dbh->selectall_arrayref('select * from mkosc.a order by i');
 is_deeply(
    $new_rows,
    $org_rows,  # from previous run since old table was dropped this run
@@ -149,6 +181,14 @@
    "Exit status 0"
 );
 
+# Bug 933232: drop temp table replicates and break replication.
+$rows = $slave_dbh->selectrow_hashref('show slave status');
+is(
+   $rows->{last_error},
+   '',
+   "Dropping temp table doesn't break replication (bug 933232)"
+);
+
 # ############################################################################
 # Alter a table with foreign keys.
 # ############################################################################
@@ -164,7 +204,7 @@
 $sb->load_file('master', "t/pt-online-schema-change/samples/fk_tables_schema.sql");
 
 # city has a fk constraint on country, so get its original table def.
-my $orig_table_def = $dbh->selectrow_hashref('show create table mkosc.city')->{'create table'};
+my $orig_table_def = $master_dbh->selectrow_hashref('show create table mkosc.city')->{'create table'};
 
 # Alter the parent table.  The error we need to avoid is:
 # DBD::mysql::db do failed: Cannot delete or update a parent row:
@@ -181,7 +221,7 @@
    "Exit status 0 (rebuild_constraints method)"
 );
 
-$rows = $dbh->selectall_arrayref('show tables from mkosc like "\_\_%"');
+$rows = $master_dbh->selectall_arrayref('show tables from mkosc like "\_\_%"');
 is_deeply(
    $rows,
    [],
@@ -193,7 +233,7 @@
 # also updated city's fk constraint to __old_country.  We should have
 # dropped and re-added that constraint exactly, changing only __old_country
 # to country, like it originally was.
-my $new_table_def = $dbh->selectrow_hashref('show create table mkosc.city')->{'create table'};
+my $new_table_def = $master_dbh->selectrow_hashref('show create table mkosc.city')->{'create table'};
 is(
    $new_table_def,
    $orig_table_def,
@@ -205,7 +245,7 @@
 #######################
 $sb->load_file('master', "t/pt-online-schema-change/samples/fk_tables_schema.sql");
 
-$orig_table_def = $dbh->selectrow_hashref('show create table mkosc.city')->{'create table'};
+$orig_table_def = $master_dbh->selectrow_hashref('show create table mkosc.city')->{'create table'};
 
 output(
    sub { $exit = pt_online_schema_change::main(@args,
@@ -218,14 +258,14 @@
    "Exit status 0 (drop_old_table method)"
 );
 
-$rows = $dbh->selectall_arrayref('show tables from mkosc like "\_\_%"');
+$rows = $master_dbh->selectall_arrayref('show tables from mkosc like "\_\_%"');
 is_deeply(
    $rows,
    [],
    "Old table dropped (drop_old_table method)"
 ) or print Dumper($rows);
 
-$new_table_def = $dbh->selectrow_hashref('show create table mkosc.city')->{'create table'};
+$new_table_def = $master_dbh->selectrow_hashref('show create table mkosc.city')->{'create table'};
 is(
    $new_table_def,
    $orig_table_def,
@@ -240,19 +280,19 @@
    my ($file, $name) = @args{qw(file name)};
 
    $sb->load_file('master', "t/lib/samples/osc/$file");
-   PerconaTest::wait_for_table($dbh, "osc.t", "id=5");
-   PerconaTest::wait_for_table($dbh, "osc.__new_t");
-   $dbh->do('use osc');
-   $dbh->do("DROP TABLE IF EXISTS osc.__new_t");
+   PerconaTest::wait_for_table($master_dbh, "osc.t", "id=5");
+   PerconaTest::wait_for_table($master_dbh, "osc.__new_t");
+   $master_dbh->do('use osc');
+   $master_dbh->do("DROP TABLE IF EXISTS osc.__new_t");
 
-   $org_rows = $dbh->selectall_arrayref('select * from osc.t order by id');
+   $org_rows = $master_dbh->selectall_arrayref('select * from osc.t order by id');
 
    output(
       sub { $exit = pt_online_schema_change::main(@args,
          'D=osc,t=t', qw(--alter ENGINE=InnoDB)) },
    );
 
-   $new_rows = $dbh->selectall_arrayref('select * from osc.t order by id');
+   $new_rows = $master_dbh->selectall_arrayref('select * from osc.t order by id');
 
    is_deeply(
       $new_rows,
@@ -280,5 +320,5 @@
 # #############################################################################
 # Done.
 # #############################################################################
-$sb->wipe_clean($dbh);
+$sb->wipe_clean($master_dbh);
 exit;

=== modified file 't/pt-online-schema-change/option_sanity.t'
--- t/pt-online-schema-change/option_sanity.t	2011-07-12 22:56:55 +0000
+++ t/pt-online-schema-change/option_sanity.t	2012-03-02 20:34:36 +0000
@@ -9,7 +9,7 @@
 use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
-use Test::More tests => 4;
+use Test::More tests => 5;
 
 use PerconaTest;
 
@@ -44,6 +44,13 @@
    "Either DSN D part or --database required"
 );
 
+$output = `$cmd --help`;
+like(
+   $output,
+   qr/--execute\s+FALSE/,
+   "--execute FALSE by default"
+);
+
 # #############################################################################
 # Done.
 # #############################################################################