From 7c76b25179280d4bd733175ca6ff322f4969d47d Mon Sep 17 00:00:00 2001 From: Daniel Nichter Date: Tue, 21 Feb 2012 12:34:21 -0700 Subject: [PATCH 1/2] Fix basics.t so it doesn't break replication. Add IF EXISTS to DROP TABLE statements. --- bin/pt-online-schema-change | 4 +- t/pt-online-schema-change/basics.t | 69 ++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index e39042ec..a908e983 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -4218,7 +4218,7 @@ sub main { 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 +4241,7 @@ sub main { $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'); } diff --git a/t/pt-online-schema-change/basics.t b/t/pt-online-schema-change/basics.t index fc9aacb1..ffec55ab 100644 --- a/t/pt-online-schema-change/basics.t +++ b/t/pt-online-schema-change/basics.t @@ -19,13 +19,17 @@ use Data::Dumper; 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 => 24; } my $output = ""; @@ -35,7 +39,9 @@ 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'); # ############################################################################# # --check-tables-and-exit @@ -86,7 +92,7 @@ output( '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 +105,8 @@ is( "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 +119,19 @@ is( "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. # ############################################################################# -$dbh->do('drop table mkosc.__old_a'); # from previous run +$master_dbh->do('drop table if exists mkosc.__old_a'); # from prev run $sb->load_file('master', "t/pt-online-schema-change/samples/small_table.sql"); output( @@ -124,7 +139,7 @@ output( '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 +151,7 @@ ok( "--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 +164,14 @@ is( "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 +187,7 @@ is( $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 +204,7 @@ is( "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 +216,7 @@ is_deeply( # 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 +228,7 @@ is( ####################### $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 +241,14 @@ is( "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 +263,19 @@ sub test_table { 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 +303,5 @@ test_table( # ############################################################################# # Done. # ############################################################################# -$sb->wipe_clean($dbh); +$sb->wipe_clean($master_dbh); exit; From 866a0424445603c52a06b3c68b2cd809110ea54c Mon Sep 17 00:00:00 2001 From: Daniel Nichter Date: Fri, 2 Mar 2012 12:30:16 -0800 Subject: [PATCH 2/2] Add --execute and die unless it's given. Enhance docu/risks about replication. --- bin/pt-online-schema-change | 44 ++++++++++++++++--- .../alter_active_table.t | 2 +- t/pt-online-schema-change/basics.t | 17 ++++++- t/pt-online-schema-change/option_sanity.t | 9 +++- 4 files changed, 61 insertions(+), 11 deletions(-) diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index a908e983..ffb786e9 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -4110,6 +4110,13 @@ sub main { # ##################################################################### # 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 = ""; @@ -4510,12 +4517,12 @@ whether known or unknown, of using this tool. The two main categories of risks 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 @@ the tool might do before actually doing it. =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 @@ swapped for the original table), then the old table can be restored. 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 diff --git a/t/pt-online-schema-change/alter_active_table.t b/t/pt-online-schema-change/alter_active_table.t index 10221dc3..fc2cc66c 100644 --- a/t/pt-online-schema-change/alter_active_table.t +++ b/t/pt-online-schema-change/alter_active_table.t @@ -33,7 +33,7 @@ else { my $output = ""; my $cnf = '/tmp/12345/my.sandbox.cnf'; -my @args = ('-F', $cnf); +my @args = ('-F', $cnf, '--execute'); my $exit = 0; my $rows; diff --git a/t/pt-online-schema-change/basics.t b/t/pt-online-schema-change/basics.t index ffec55ab..db8153ac 100644 --- a/t/pt-online-schema-change/basics.t +++ b/t/pt-online-schema-change/basics.t @@ -29,12 +29,12 @@ elsif ( !$slave_dbh ) { plan skip_all => 'Cannot connect to sandbox slave'; } else { - plan tests => 24; + 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; @@ -43,6 +43,19 @@ 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 # ############################################################################# diff --git a/t/pt-online-schema-change/option_sanity.t b/t/pt-online-schema-change/option_sanity.t index ed8fe423..628b6a78 100644 --- a/t/pt-online-schema-change/option_sanity.t +++ b/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 => 4; +use Test::More tests => 5; use PerconaTest; @@ -44,6 +44,13 @@ like( "Either DSN D part or --database required" ); +$output = `$cmd --help`; +like( + $output, + qr/--execute\s+FALSE/, + "--execute FALSE by default" +); + # ############################################################################# # Done. # #############################################################################