Merge lp:~percona-toolkit-dev/percona-toolkit/preserve-foreign-keys-bug-969726 into lp:~percona-toolkit-dev/percona-toolkit/pt-osc-2.1.1

[Daniel Nichter]

[Daniel Nichter (daniel-nichter)]

review: Approve

=== modified file 'bin/pt-online-schema-change'
--- bin/pt-online-schema-change	2012-03-29 23:10:30 +0000
+++ bin/pt-online-schema-change	2012-04-03 00:15:25 +0000
@@ -5236,8 +5236,17 @@
    );
    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 @@
          Cxn          => $cxn,
          Quoter       => $q,
          OptionParser => $o,
+         TableParser  => $tp,
       );
    };
    if ( $EVAL_ERROR ) {
@@ -5966,11 +5976,18 @@
 # ############################################################################
 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 @@
    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 <database>.<constraint> 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 @@
 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 @@
 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
 

=== removed symlink 't/lib/bash/alt_cmds.t'
=== target was u'../../../util/test-bash-functions'
=== removed symlink 't/lib/bash/collect.t'
=== target was u'../../../util/test-bash-functions'
=== removed symlink 't/lib/bash/daemon.t'
=== target was u'../../../util/test-bash-functions'
=== removed symlink 't/lib/bash/log_warn_die.t'
=== target was u'../../../util/test-bash-functions'
=== removed symlink 't/lib/bash/parse_options.t'
=== target was u'../../../util/test-bash-functions'
=== removed symlink 't/lib/bash/safeguards.t'
=== target was u'../../../util/test-bash-functions'
=== removed symlink 't/lib/bash/tmpdir.t'
=== target was u'../../../util/test-bash-functions'
=== modified file 't/pt-online-schema-change/basics.t'
--- t/pt-online-schema-change/basics.t	2012-03-29 22:53:11 +0000
+++ t/pt-online-schema-change/basics.t	2012-04-03 00:15:25 +0000
@@ -32,7 +32,7 @@
    plan skip_all => 'Cannot connect to sandbox slave';
 }
 else {
-   plan tests => 90;
+   plan tests => 105;
 }
 
 my $q      = new Quoter();
@@ -90,6 +90,7 @@
    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 @@
          # 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 @@
    if ( $fail ) {
       diag("Output from failed test:\n$output");
    }
+   elsif ( $args{output} ) {
+      warn $output;
+   }
 
    return;
 }
@@ -455,6 +461,45 @@
    ],
 );
 
+# 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 @@
          '--alter', 'ENGINE=InnoDB'
       ],
    );
+
+   # Restore the original fks.
+   diag(`$trunk/sandbox/load-sakila-db 12345`);
 }
 
 # #############################################################################
@@ -549,7 +597,7 @@
       0,
       "$name exit status 0"
    ) or $fail = 1;
-   
+
    if ( $fail ) {
       diag("Output from failed test:\n$output");
    }