From 7ec6abd6849e0e5b934b83ba763658aec3f05d59 Mon Sep 17 00:00:00 2001 From: Daniel Nichter Date: Tue, 29 Jan 2013 17:51:43 -0700 Subject: [PATCH 1/3] Fall back to using pk/ui from orig table if new table del index uses cols that no longer exist in orig table. --- bin/pt-online-schema-change | 76 ++++++++++++++++--- t/pt-online-schema-change/bugs.t | 28 +++++++ .../samples/del-trg-bug-1103672.sql | 20 +++++ 3 files changed, 114 insertions(+), 10 deletions(-) create mode 100644 t/pt-online-schema-change/samples/del-trg-bug-1103672.sql diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index 9ec01041..abaf238c 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -8370,19 +8370,74 @@ sub main { # Find a pk or unique index to use for the delete trigger. can_nibble() # above returns an index, but NibbleIterator will use non-unique indexes, # so we have to do this again here. - my $indexes = $new_tbl->{tbl_struct}->{keys}; # brevity - foreach my $index ( $tp->sort_indexes($new_tbl->{tbl_struct}) ) { - if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) { - PTDEBUG && _d('Delete trigger index:', Dumper($index)); - $new_tbl->{del_index} = $index; - last; + { + my $indexes = $new_tbl->{tbl_struct}->{keys}; # brevity + foreach my $index ( $tp->sort_indexes($new_tbl->{tbl_struct}) ) { + if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) { + PTDEBUG && _d('Delete trigger new index:', Dumper($index)); + $new_tbl->{del_index} = $index; + last; + } } + PTDEBUG && _d('New table delete index:', $new_tbl->{del_index}); } + + { + my $indexes = $orig_tbl->{tbl_struct}->{keys}; # brevity + foreach my $index ( $tp->sort_indexes($orig_tbl->{tbl_struct}) ) { + if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) { + PTDEBUG && _d('Delete trigger orig index:', Dumper($index)); + $orig_tbl->{del_index} = $index; + last; + } + } + PTDEBUG && _d('Orig table delete index:', $orig_tbl->{del_index}); + } + if ( !$new_tbl->{del_index} ) { die "The new table $new_tbl->{name} does not have a PRIMARY KEY " . "or a unique index which is required for the DELETE trigger.\n"; } + # Determine whether to use the new or orig table delete index. + # The new table del index is preferred due to + # https://bugs.launchpad.net/percona-toolkit/+bug/1062324 + # In short, if the chosen del index is re-created with new columns, + # its original columns may be dropped, so just use its new columns. + # But, due to https://bugs.launchpad.net/percona-toolkit/+bug/1103672, + # the chosen del index on the new table may reference columns which + # do not/no longer exist in the orig table, so we check for this + # and, if it's the case, we fall back to using the del index from + # the orig table. + my $del_tbl = $new_tbl; # preferred + my $new_del_index_cols # brevity + = $new_tbl->{tbl_struct}->{keys}->{ $new_tbl->{del_index} }->{cols}; + foreach my $new_del_index_col ( @$new_del_index_cols ) { + if ( !exists $orig_cols->{$new_del_index_col} ) { + if ( !$orig_tbl->{del_index} ) { + die "The new table index $new_tbl->{del_index} would be used " + . "for the DELETE trigger, but it uses column " + . "$new_del_index_col which does not exist in the original " + . "table and the original table does not have a PRIMARY KEY " + . "or a unique index to use for the DELETE trigger.\n"; + } + print "Using original table index $orig_tbl->{del_index} for the " + . "DELETE trigger instead of new table index $new_tbl->{del_index} " + . "because the new table index uses column $new_del_index_col " + . "which does not exist in the original table.\n"; + $del_tbl = $orig_tbl; + last; + } + } + + { + my $del_cols + = $del_tbl->{tbl_struct}->{keys}->{ $del_tbl->{del_index} }->{cols}; + PTDEBUG && _d('Index for delete trigger: table', $del_tbl->{name}, + 'index', $del_tbl->{del_index}, + 'columns', @$del_cols); + } + # ######################################################################## # Step 3: Create the triggers to capture changes on the original table and # apply them to the new table. @@ -8412,6 +8467,7 @@ sub main { create_triggers( orig_tbl => $orig_tbl, new_tbl => $new_tbl, + del_tbl => $del_tbl, columns => \@common_cols, Cxn => $cxn, Quoter => $q, @@ -9536,11 +9592,11 @@ sub drop_swap { sub create_triggers { my ( %args ) = @_; - my @required_args = qw(orig_tbl new_tbl columns Cxn Quoter OptionParser); + my @required_args = qw(orig_tbl new_tbl del_tbl columns Cxn Quoter OptionParser); foreach my $arg ( @required_args ) { die "I need a $arg argument" unless $args{$arg}; } - my ($orig_tbl, $new_tbl, $cols, $cxn, $q, $o) = @args{@required_args}; + my ($orig_tbl, $new_tbl, $del_tbl, $cols, $cxn, $q, $o) = @args{@required_args}; # This sub works for --dry-run and --execute. With --dry-run it's # only interesting if --print is specified, too; then the user can @@ -9569,8 +9625,8 @@ sub create_triggers { # unique indexes can be nullable. Cols are from the new table and # they may have been renamed my %old_col_for = map { $_->{new} => $_->{old} } @$cols; - my $tbl_struct = $new_tbl->{tbl_struct}; - my $del_index = $new_tbl->{del_index}; + my $tbl_struct = $del_tbl->{tbl_struct}; + my $del_index = $del_tbl->{del_index}; my $del_index_cols = join(" AND ", map { my $new_col = $_; my $old_col = $old_col_for{$new_col} || $new_col; diff --git a/t/pt-online-schema-change/bugs.t b/t/pt-online-schema-change/bugs.t index 58b1b567..9d4afc48 100644 --- a/t/pt-online-schema-change/bugs.t +++ b/t/pt-online-schema-change/bugs.t @@ -233,6 +233,34 @@ $sb->load_file('master', "$sample/del-trg-bug-1062324.sql"); undef, "Delete trigger works after altering PK (bug 1062324)" ); + + # Another instance of this bug: + # https://bugs.launchpad.net/percona-toolkit/+bug/1103672 + $sb->load_file('master', "$sample/del-trg-bug-1103672.sql"); + + ($output, $exit_status) = full_output( + sub { pt_online_schema_change::main(@args, + "$master_dsn,D=test,t=t1", + "--alter", "drop primary key, add column _id int unsigned not null primary key auto_increment FIRST", + qw(--execute --no-drop-new-table --no-swap-tables)) }, + ); + + eval { + $master_dbh->do("DELETE FROM test.t1 WHERE id=1"); + }; + is( + $EVAL_ERROR, + "", + "No delete trigger error after altering PK (bug 1103672)" + ) or diag($output); + + $row = $master_dbh->selectrow_arrayref("SELECT * FROM test._t1_new WHERE id=1"); + is( + $row, + undef, + "Delete trigger works after altering PK (bug 1103672)" + ); + } # ############################################################################# diff --git a/t/pt-online-schema-change/samples/del-trg-bug-1103672.sql b/t/pt-online-schema-change/samples/del-trg-bug-1103672.sql new file mode 100644 index 00000000..a5d7699e --- /dev/null +++ b/t/pt-online-schema-change/samples/del-trg-bug-1103672.sql @@ -0,0 +1,20 @@ +drop database if exists test; +create database test; +use test; + +CREATE TABLE `t1` ( + `id` int(10) unsigned NOT NULL, + `x` char(3) DEFAULT NULL, + PRIMARY KEY (`id`) +) ENGINE=InnoDB; + +INSERT INTO t1 VALUES + (1, 'a'), + (2, 'b'), + (3, 'c'), + (4, 'd'), + (5, 'f'), + (6, 'g'), + (7, 'h'), + (8, 'i'), + (9, 'j'); From 680477ed581f26acf73da38819c45afe19b552d4 Mon Sep 17 00:00:00 2001 From: Daniel Nichter Date: Tue, 29 Jan 2013 18:16:38 -0700 Subject: [PATCH 2/3] Add 'DROP PRIMARY KEY' to --check-alter. --- bin/pt-online-schema-change | 32 ++++++++++++ t/pt-online-schema-change/bugs.t | 4 +- t/pt-online-schema-change/check_alter.t | 65 +++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 t/pt-online-schema-change/check_alter.t diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index abaf238c..9da33704 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -8986,6 +8986,28 @@ sub check_alter { my $ok = 1; + # ######################################################################## + # Check for DROP PRIMARY KEY. + # ######################################################################## + if ( $alter =~ m/DROP\s+PRIMARY\s+KEY/i ) { + my $msg = "--alter contains 'DROP PRIMARY KEY'. Dropping and " + . "altering the primary key can be very dangerous, " + . "especially if the original table does not have other " + . "unique indexes.\n"; + if ( $dry_run ) { + print $msg; + } + else { + $ok = 0; + warn $msg + . "The tool should handle this correctly, but you should " + . "test it first and carefully examine the triggers which " + . "rely on the PRIMARY KEY or a unique index. Specify " + . "--no-check-alter to disable this check and perform the " + . "--alter.\n"; + } + } + # ######################################################################## # Check for renamed columns. # https://bugs.launchpad.net/percona-toolkit/+bug/1068562 @@ -9032,6 +9054,7 @@ sub check_alter { } if ( !$ok ) { + # check_alter.t relies on this output. die "--check-alter failed.\n"; } @@ -10286,6 +10309,15 @@ code that does this is not a full-blown SQL parser, so we recommend that users run the tool with L<"--dry-run"> and check if it's detecting the renames correctly. +=item DROP PRIMARY KEY + +If L<"--alter"> contain C (case- and space-insensitive), +a warning is printed and the tool exits unless L<"--dry-run"> is specified. +Altering the primary key can be very dangerous, but the tool can handle it. +The tool's triggers, particularly the DELETE trigger, are most affected by +altering the primary key because the tool prefers to use the primary key +for its triggers. + =back =item --[no]check-plan diff --git a/t/pt-online-schema-change/bugs.t b/t/pt-online-schema-change/bugs.t index 9d4afc48..16fce594 100644 --- a/t/pt-online-schema-change/bugs.t +++ b/t/pt-online-schema-change/bugs.t @@ -210,7 +210,7 @@ $sb->load_file('master', "$sample/del-trg-bug-1062324.sql"); sub { pt_online_schema_change::main(@args, "$master_dsn,D=test,t=t1", "--alter", "drop key 2bpk, drop key c3, drop primary key, drop c1, add primary key (c2, c3(4)), add key (c3(4))", - qw(--execute --no-drop-new-table --no-swap-tables)) }, + qw(--no-check-alter --execute --no-drop-new-table --no-swap-tables)) }, ); # Since _t1_new no longer has the c1 column, the bug caused this @@ -242,7 +242,7 @@ $sb->load_file('master', "$sample/del-trg-bug-1062324.sql"); sub { pt_online_schema_change::main(@args, "$master_dsn,D=test,t=t1", "--alter", "drop primary key, add column _id int unsigned not null primary key auto_increment FIRST", - qw(--execute --no-drop-new-table --no-swap-tables)) }, + qw(--no-check-alter --execute --no-drop-new-table --no-swap-tables)) }, ); eval { diff --git a/t/pt-online-schema-change/check_alter.t b/t/pt-online-schema-change/check_alter.t new file mode 100644 index 00000000..7afd3c97 --- /dev/null +++ b/t/pt-online-schema-change/check_alter.t @@ -0,0 +1,65 @@ +#!/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 Data::Dumper; +use PerconaTest; +use Sandbox; + +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 $slave_dbh = $sb->get_dbh_for('slave1'); + +if ( !$master_dbh ) { + plan skip_all => 'Cannot connect to sandbox master'; +} +elsif ( !$slave_dbh ) { + plan skip_all => 'Cannot connect to sandbox slave1'; +} + +# The sandbox servers run with lock_wait_timeout=3 and it's not dynamic +# so we need to specify --lock-wait-timeout=3 else the tool will die. +my $master_dsn = 'h=127.1,P=12345,u=msandbox,p=msandbox'; +my @args = (qw(--lock-wait-timeout 3)); +my $output; +my $exit_status; +my $sample = "t/pt-online-schema-change/samples/"; + +# ############################################################################# +# DROP PRIMARY KEY +# ############################################################################# + +$sb->load_file('master', "$sample/del-trg-bug-1103672.sql"); + +($output, $exit_status) = full_output( + sub { pt_online_schema_change::main(@args, + "$master_dsn,D=test,t=t1", + "--alter", "drop primary key, add column _id int unsigned not null primary key auto_increment FIRST", + qw(--execute)), + }, +); + +like( + $output, + qr/--check-alter failed/, + "DROP PRIMARY KEY" +); + +# ############################################################################# +# Done. +# ############################################################################# +$sb->wipe_clean($master_dbh); +ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox"); +done_testing; From 93146ce58a629363afacf3e7c8f8c48fea6b65c8 Mon Sep 17 00:00:00 2001 From: Daniel Nichter Date: Wed, 30 Jan 2013 08:04:19 -0700 Subject: [PATCH 3/3] Soften the warning about 'drop primary key' a little. --- bin/pt-online-schema-change | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index 9da33704..7aa7a139 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -8991,7 +8991,7 @@ sub check_alter { # ######################################################################## if ( $alter =~ m/DROP\s+PRIMARY\s+KEY/i ) { my $msg = "--alter contains 'DROP PRIMARY KEY'. Dropping and " - . "altering the primary key can be very dangerous, " + . "altering the primary key can be dangerous, " . "especially if the original table does not have other " . "unique indexes.\n"; if ( $dry_run ) { @@ -10305,18 +10305,19 @@ In previous versions of the tool, renaming a column with C would lead to that column's data being lost. The tool now parses the alter statement and tries to catch these cases, so the renamed columns should have the same data as the originals. However, the -code that does this is not a full-blown SQL parser, so we recommend that users -run the tool with L<"--dry-run"> and check if it's detecting the renames -correctly. +code that does this is not a full-blown SQL parser, so you should first +run the tool with L<"--dry-run"> and L<"--print"> and verify that it detects +the renamed columns correctly. =item DROP PRIMARY KEY If L<"--alter"> contain C (case- and space-insensitive), a warning is printed and the tool exits unless L<"--dry-run"> is specified. -Altering the primary key can be very dangerous, but the tool can handle it. +Altering the primary key can be dangerous, but the tool can handle it. The tool's triggers, particularly the DELETE trigger, are most affected by altering the primary key because the tool prefers to use the primary key -for its triggers. +for its triggers. You should first run the tool with L<"--dry-run"> and +L<"--print"> and verify that the triggers are correct. =back