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

Proposed by Daniel Nichter on 2012-04-03
Status: Merged
Approved by: Daniel Nichter on 2012-04-03
Approved revision: 252
Merged at revision: 250
Proposed branch: lp:~percona-toolkit-dev/percona-toolkit/preserve-foreign-keys-bug-969726
Merge into: lp:~percona-toolkit-dev/percona-toolkit/pt-osc-2.1.1
Diff against target: 226 lines (+96/-11)
2 files modified
bin/pt-online-schema-change (+44/-7)
t/pt-online-schema-change/basics.t (+52/-4)
To merge this branch: bzr merge lp:~percona-toolkit-dev/percona-toolkit/preserve-foreign-keys-bug-969726
Reviewer Review Type Date Requested Status
Daniel Nichter Approve on 2012-04-03
Review via email: mp+100537@code.launchpad.net
To post a comment you must log in.
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/pt-online-schema-change'
2--- bin/pt-online-schema-change 2012-03-29 23:10:30 +0000
3+++ bin/pt-online-schema-change 2012-04-03 00:15:25 +0000
4@@ -5236,8 +5236,17 @@
5 );
6 if ( !$child_tables ) {
7 if ( $alter_fk_method ) {
8- warn "No foreign keys reference the table; ignoring "
9+ warn "No foreign keys reference $orig_tbl->{name}; ignoring "
10 . "--alter-foreign-keys-method.\n";
11+
12+ if ( $alter_fk_method eq 'drop_swap' ) {
13+ # These opts are disabled at the start if the user specifies
14+ # the drop_swap method, but now that we know there are no
15+ # child tables, we must re-enable these to make the alter work.
16+ $o->set('swap-tables', 1);
17+ $o->set('drop-old-table', 1);
18+ }
19+
20 $alter_fk_method = '';
21 }
22 # No child tables and --alter-fk-method wasn't specified,
23@@ -5355,6 +5364,7 @@
24 Cxn => $cxn,
25 Quoter => $q,
26 OptionParser => $o,
27+ TableParser => $tp,
28 );
29 };
30 if ( $EVAL_ERROR ) {
31@@ -5966,11 +5976,18 @@
32 # ############################################################################
33 sub create_new_table{
34 my (%args) = @_;
35- my @required_args = qw(orig_tbl Cxn Quoter OptionParser);
36+ my @required_args = qw(orig_tbl Cxn Quoter OptionParser TableParser);
37 foreach my $arg ( @required_args ) {
38 die "I need a $arg argument" unless $args{$arg};
39 }
40- my ($orig_tbl, $cxn, $q, $o) = @args{@required_args};
41+ my ($orig_tbl, $cxn, $q, $o, $tp) = @args{@required_args};
42+
43+ # Get the original table struct.
44+ my $ddl = $tp->get_create_table(
45+ $cxn->dbh(),
46+ $orig_tbl->{db},
47+ $orig_tbl->{tbl},
48+ );
49
50 my $tries = $args{tries} || 10; # don't try forever
51 my $prefix = $args{prefix} || '_';
52@@ -5982,8 +5999,22 @@
53 my @old_tables;
54 while ( $tryno++ < $tries ) {
55 $table_name = $prefix . $table_name;
56- my $sql = "CREATE TABLE " . $q->quote($orig_tbl->{db}, $table_name)
57- . " LIKE $orig_tbl->{name}";
58+ my $quoted = $q->quote($orig_tbl->{db}, $table_name);
59+
60+ # Generate SQL to create the new table. We do not use CREATE TABLE LIKE
61+ # because it doesn't preserve foreign key constraints. Here we need to
62+ # rename the FK constraints, too. This is because FK constraints are
63+ # internally stored as <database>.<constraint> and there cannot be
64+ # duplicates. If we don't rename the constraints, then InnoDB will throw
65+ # error 121 (duplicate key violation) when we try to execute the CREATE
66+ # TABLE. TODO: this code isn't perfect. If we rename a constraint from
67+ # foo to _foo and there is already a constraint with that name in this
68+ # or another table, we can still have a collision. But if there are
69+ # multiple FKs on this table, it's hard to know which one is causing the
70+ # trouble. Should we generate random/UUID FK names or something instead?
71+ my $sql = $ddl;
72+ $sql =~ s/\ACREATE TABLE .*?\($/CREATE TABLE $quoted (/m;
73+ $sql =~ s/^ CONSTRAINT `/ CONSTRAINT `_/gm;
74 PTDEBUG && _d($sql);
75 eval {
76 $cxn->dbh()->do($sql);
77@@ -6766,6 +6797,11 @@
78 for accomplishing this. You can read more about this in the documentation for
79 L<"--alter-foreign-keys-method">.
80
81+Foreign keys also cause some side effects. The final table will have the same
82+foreign keys and indexes as the original table (unless you specify differently
83+in your ALTER statement), but the names of the objects may be changed slightly
84+to avoid object name collisions in MySQL and InnoDB.
85+
86 For safety, the tool does not modify the table unless you specify the
87 L<"--execute"> option, which is not enabled by default. The tool supports a
88 variety of other measures to prevent unwanted load or other problems, including
89@@ -6862,8 +6898,9 @@
90 much faster than the external process of copying rows.
91
92 Due to a limitation in MySQL, foreign keys will not have the same names after
93-the ALTER that they did prior to it. The tool has to rename the foreign key when
94-it redefines it, which adds a leading underscore to the name.
95+the ALTER that they did prior to it. The tool has to rename the foreign key
96+when it redefines it, which adds a leading underscore to the name. In some
97+cases, MySQL also automatically renames indexes required for the foreign key.
98
99 =item drop_swap
100
101
102=== removed symlink 't/lib/bash/alt_cmds.t'
103=== target was u'../../../util/test-bash-functions'
104=== removed symlink 't/lib/bash/collect.t'
105=== target was u'../../../util/test-bash-functions'
106=== removed symlink 't/lib/bash/daemon.t'
107=== target was u'../../../util/test-bash-functions'
108=== removed symlink 't/lib/bash/log_warn_die.t'
109=== target was u'../../../util/test-bash-functions'
110=== removed symlink 't/lib/bash/parse_options.t'
111=== target was u'../../../util/test-bash-functions'
112=== removed symlink 't/lib/bash/safeguards.t'
113=== target was u'../../../util/test-bash-functions'
114=== removed symlink 't/lib/bash/tmpdir.t'
115=== target was u'../../../util/test-bash-functions'
116=== modified file 't/pt-online-schema-change/basics.t'
117--- t/pt-online-schema-change/basics.t 2012-03-29 22:53:11 +0000
118+++ t/pt-online-schema-change/basics.t 2012-04-03 00:15:25 +0000
119@@ -32,7 +32,7 @@
120 plan skip_all => 'Cannot connect to sandbox slave';
121 }
122 else {
123- plan tests => 90;
124+ plan tests => 105;
125 }
126
127 my $q = new Quoter();
128@@ -90,6 +90,7 @@
129 my ($name, $table, $test_type, $cmds) = @args{@required_args};
130
131 my ($db, $tbl) = $q->split_unquote($table);
132+ my $table_name = $tbl;
133 my $pk_col = $args{pk_col} || 'id';
134
135 if ( my $file = $args{file} ) {
136@@ -233,11 +234,13 @@
137 # The tool does not use the same/original fk name,
138 # it appends a single _. So we need to strip this
139 # to compare the original fks to the new fks.
140- if ( $fk_method eq 'rebuild_constraints' ) {
141+ # if ( $fk_method eq 'rebuild_constraints' ) {
142+ if ( $fk_method eq 'rebuild_constraints'
143+ || $table_name eq $tbl->[0] ) {
144 my %new_fks = map {
145 my $real_fk_name = $_;
146 my $fk_name = $_;
147- if ( $fk_name =~ s/^_// ) {
148+ if ( $fk_name =~ s/^_// && $table_name ne $tbl->[0] ) {
149 $rebuild_method = 1;
150 }
151 $fks->{$real_fk_name}->{name} =~ s/^_//;
152@@ -284,6 +287,9 @@
153 if ( $fail ) {
154 diag("Output from failed test:\n$output");
155 }
156+ elsif ( $args{output} ) {
157+ warn $output;
158+ }
159
160 return;
161 }
162@@ -455,6 +461,45 @@
163 ],
164 );
165
166+# Use drop_swap to alter address, which no other table references,
167+# so the tool should re-enable --swap-tables and --drop-old-table.
168+test_alter_table(
169+ name => "Drop-swap child",
170+ table => "pt_osc.address",
171+ pk_col => "address_id",
172+ file => "basic_with_fks.sql",
173+ wait => ["pt_osc.address", "address_id=5"],
174+ test_type => "drop_col",
175+ drop_col => "last_update",
176+ cmds => [
177+ qw(
178+ --execute
179+ --alter-foreign-keys-method drop_swap
180+ ),
181+ '--alter', 'DROP COLUMN last_update',
182+ ],
183+);
184+
185+# Alter city and verify that its fk to country still exists.
186+# (https://bugs.launchpad.net/percona-toolkit/+bug/969726)
187+test_alter_table(
188+ name => "Preserve all fks",
189+ table => "pt_osc.city",
190+ pk_col => "city_id",
191+ file => "basic_with_fks.sql",
192+ wait => ["pt_osc.address", "address_id=5"],
193+ test_type => "drop_col",
194+ drop_col => "last_update",
195+ check_fks => "rebuild_constraints",
196+ cmds => [
197+ qw(
198+ --execute
199+ --alter-foreign-keys-method rebuild_constraints
200+ ),
201+ '--alter', 'DROP COLUMN last_update',
202+ ],
203+);
204+
205 SKIP: {
206 skip 'Sandbox master does not have the sakila database', 7
207 unless @{$master_dbh->selectcol_arrayref('SHOW DATABASES LIKE "sakila"')};
208@@ -478,6 +523,9 @@
209 '--alter', 'ENGINE=InnoDB'
210 ],
211 );
212+
213+ # Restore the original fks.
214+ diag(`$trunk/sandbox/load-sakila-db 12345`);
215 }
216
217 # #############################################################################
218@@ -549,7 +597,7 @@
219 0,
220 "$name exit status 0"
221 ) or $fail = 1;
222-
223+
224 if ( $fail ) {
225 diag("Output from failed test:\n$output");
226 }

Subscribers

People subscribed via source and target branches