From 02e9d2eed9549284c5ac61baefd7fa4530d88229 Mon Sep 17 00:00:00 2001 From: Daniel Nichter Date: Mon, 2 Apr 2012 18:22:51 -0600 Subject: [PATCH] Merge preserve-foreign-keys-bug-969726. --- bin/pt-online-schema-change | 51 +++++++++++++++++++++++---- t/pt-online-schema-change/basics.t | 56 +++++++++++++++++++++++++++--- 2 files changed, 96 insertions(+), 11 deletions(-) diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index 2107256b..428ff655 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -5236,8 +5236,17 @@ sub main { ); if ( !$child_tables ) { if ( $alter_fk_method ) { - warn "No foreign keys reference the table; ignoring " + warn "No foreign keys reference $orig_tbl->{name}; ignoring " . "--alter-foreign-keys-method.\n"; + + if ( $alter_fk_method eq 'drop_swap' ) { + # These opts are disabled at the start if the user specifies + # the drop_swap method, but now that we know there are no + # child tables, we must re-enable these to make the alter work. + $o->set('swap-tables', 1); + $o->set('drop-old-table', 1); + } + $alter_fk_method = ''; } # No child tables and --alter-fk-method wasn't specified, @@ -5355,6 +5364,7 @@ sub main { Cxn => $cxn, Quoter => $q, OptionParser => $o, + TableParser => $tp, ); }; if ( $EVAL_ERROR ) { @@ -5966,11 +5976,18 @@ sub main { # ############################################################################ sub create_new_table{ my (%args) = @_; - my @required_args = qw(orig_tbl Cxn Quoter OptionParser); + my @required_args = qw(orig_tbl Cxn Quoter OptionParser TableParser); foreach my $arg ( @required_args ) { die "I need a $arg argument" unless $args{$arg}; } - my ($orig_tbl, $cxn, $q, $o) = @args{@required_args}; + my ($orig_tbl, $cxn, $q, $o, $tp) = @args{@required_args}; + + # Get the original table struct. + my $ddl = $tp->get_create_table( + $cxn->dbh(), + $orig_tbl->{db}, + $orig_tbl->{tbl}, + ); my $tries = $args{tries} || 10; # don't try forever my $prefix = $args{prefix} || '_'; @@ -5982,8 +5999,22 @@ sub create_new_table{ my @old_tables; while ( $tryno++ < $tries ) { $table_name = $prefix . $table_name; - my $sql = "CREATE TABLE " . $q->quote($orig_tbl->{db}, $table_name) - . " LIKE $orig_tbl->{name}"; + my $quoted = $q->quote($orig_tbl->{db}, $table_name); + + # Generate SQL to create the new table. We do not use CREATE TABLE LIKE + # because it doesn't preserve foreign key constraints. Here we need to + # rename the FK constraints, too. This is because FK constraints are + # internally stored as . and there cannot be + # duplicates. If we don't rename the constraints, then InnoDB will throw + # error 121 (duplicate key violation) when we try to execute the CREATE + # TABLE. TODO: this code isn't perfect. If we rename a constraint from + # foo to _foo and there is already a constraint with that name in this + # or another table, we can still have a collision. But if there are + # multiple FKs on this table, it's hard to know which one is causing the + # trouble. Should we generate random/UUID FK names or something instead? + my $sql = $ddl; + $sql =~ s/\ACREATE TABLE .*?\($/CREATE TABLE $quoted (/m; + $sql =~ s/^ CONSTRAINT `/ CONSTRAINT `_/gm; PTDEBUG && _d($sql); eval { $cxn->dbh()->do($sql); @@ -6766,6 +6797,11 @@ the new table after the schema change is complete. The tool supports two methods for accomplishing this. You can read more about this in the documentation for L<"--alter-foreign-keys-method">. +Foreign keys also cause some side effects. The final table will have the same +foreign keys and indexes as the original table (unless you specify differently +in your ALTER statement), but the names of the objects may be changed slightly +to avoid object name collisions in MySQL and InnoDB. + For safety, the tool does not modify the table unless you specify the L<"--execute"> option, which is not enabled by default. The tool supports a variety of other measures to prevent unwanted load or other problems, including @@ -6862,8 +6898,9 @@ rate by L<"--chunk-size-limit">, because MySQL's C is typically much faster than the external process of copying rows. Due to a limitation in MySQL, foreign keys will not have the same names after -the ALTER that they did prior to it. The tool has to rename the foreign key when -it redefines it, which adds a leading underscore to the name. +the ALTER that they did prior to it. The tool has to rename the foreign key +when it redefines it, which adds a leading underscore to the name. In some +cases, MySQL also automatically renames indexes required for the foreign key. =item drop_swap diff --git a/t/pt-online-schema-change/basics.t b/t/pt-online-schema-change/basics.t index e2ea6d73..7ca9b699 100644 --- a/t/pt-online-schema-change/basics.t +++ b/t/pt-online-schema-change/basics.t @@ -32,7 +32,7 @@ elsif ( !$slave_dbh ) { plan skip_all => 'Cannot connect to sandbox slave'; } else { - plan tests => 90; + plan tests => 105; } my $q = new Quoter(); @@ -90,6 +90,7 @@ sub test_alter_table { my ($name, $table, $test_type, $cmds) = @args{@required_args}; my ($db, $tbl) = $q->split_unquote($table); + my $table_name = $tbl; my $pk_col = $args{pk_col} || 'id'; if ( my $file = $args{file} ) { @@ -233,11 +234,13 @@ sub test_alter_table { # The tool does not use the same/original fk name, # it appends a single _. So we need to strip this # to compare the original fks to the new fks. - if ( $fk_method eq 'rebuild_constraints' ) { + # if ( $fk_method eq 'rebuild_constraints' ) { + if ( $fk_method eq 'rebuild_constraints' + || $table_name eq $tbl->[0] ) { my %new_fks = map { my $real_fk_name = $_; my $fk_name = $_; - if ( $fk_name =~ s/^_// ) { + if ( $fk_name =~ s/^_// && $table_name ne $tbl->[0] ) { $rebuild_method = 1; } $fks->{$real_fk_name}->{name} =~ s/^_//; @@ -284,6 +287,9 @@ sub test_alter_table { if ( $fail ) { diag("Output from failed test:\n$output"); } + elsif ( $args{output} ) { + warn $output; + } return; } @@ -455,6 +461,45 @@ test_alter_table( ], ); +# Use drop_swap to alter address, which no other table references, +# so the tool should re-enable --swap-tables and --drop-old-table. +test_alter_table( + name => "Drop-swap child", + table => "pt_osc.address", + pk_col => "address_id", + file => "basic_with_fks.sql", + wait => ["pt_osc.address", "address_id=5"], + test_type => "drop_col", + drop_col => "last_update", + cmds => [ + qw( + --execute + --alter-foreign-keys-method drop_swap + ), + '--alter', 'DROP COLUMN last_update', + ], +); + +# Alter city and verify that its fk to country still exists. +# (https://bugs.launchpad.net/percona-toolkit/+bug/969726) +test_alter_table( + name => "Preserve all fks", + table => "pt_osc.city", + pk_col => "city_id", + file => "basic_with_fks.sql", + wait => ["pt_osc.address", "address_id=5"], + test_type => "drop_col", + drop_col => "last_update", + check_fks => "rebuild_constraints", + cmds => [ + qw( + --execute + --alter-foreign-keys-method rebuild_constraints + ), + '--alter', 'DROP COLUMN last_update', + ], +); + SKIP: { skip 'Sandbox master does not have the sakila database', 7 unless @{$master_dbh->selectcol_arrayref('SHOW DATABASES LIKE "sakila"')}; @@ -478,6 +523,9 @@ SKIP: { '--alter', 'ENGINE=InnoDB' ], ); + + # Restore the original fks. + diag(`$trunk/sandbox/load-sakila-db 12345`); } # ############################################################################# @@ -549,7 +597,7 @@ sub test_table { 0, "$name exit status 0" ) or $fail = 1; - + if ( $fail ) { diag("Output from failed test:\n$output"); }