Merge lp:~percona-toolkit-dev/percona-toolkit/pk-del-bug-1103672-encore into lp:percona-toolkit/2.1
- pk-del-bug-1103672-encore
- Merge into 2.1
Proposed by
Daniel Nichter
Status: | Merged |
---|---|
Merged at revision: | 523 |
Proposed branch: | lp:~percona-toolkit-dev/percona-toolkit/pk-del-bug-1103672-encore |
Merge into: | lp:percona-toolkit/2.1 |
Diff against target: |
322 lines (+218/-16) 4 files modified
bin/pt-online-schema-change (+104/-15) t/pt-online-schema-change/bugs.t (+29/-1) t/pt-online-schema-change/check_alter.t (+65/-0) t/pt-online-schema-change/samples/del-trg-bug-1103672.sql (+20/-0) |
To merge this branch: | bzr merge lp:~percona-toolkit-dev/percona-toolkit/pk-del-bug-1103672-encore |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Daniel Nichter | Approve | ||
Review via email: mp+145527@code.launchpad.net |
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
Daniel Nichter (daniel-nichter) : | # |
review:
Approve
- 523. By Daniel Nichter
-
Soften the warning about 'drop primary key' a little.
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 2013-01-03 00:54:18 +0000 | |||
3 | +++ bin/pt-online-schema-change 2013-01-30 15:06:22 +0000 | |||
4 | @@ -8370,19 +8370,74 @@ | |||
5 | 8370 | # Find a pk or unique index to use for the delete trigger. can_nibble() | 8370 | # Find a pk or unique index to use for the delete trigger. can_nibble() |
6 | 8371 | # above returns an index, but NibbleIterator will use non-unique indexes, | 8371 | # above returns an index, but NibbleIterator will use non-unique indexes, |
7 | 8372 | # so we have to do this again here. | 8372 | # so we have to do this again here. |
16 | 8373 | my $indexes = $new_tbl->{tbl_struct}->{keys}; # brevity | 8373 | { |
17 | 8374 | foreach my $index ( $tp->sort_indexes($new_tbl->{tbl_struct}) ) { | 8374 | my $indexes = $new_tbl->{tbl_struct}->{keys}; # brevity |
18 | 8375 | if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) { | 8375 | foreach my $index ( $tp->sort_indexes($new_tbl->{tbl_struct}) ) { |
19 | 8376 | PTDEBUG && _d('Delete trigger index:', Dumper($index)); | 8376 | if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) { |
20 | 8377 | $new_tbl->{del_index} = $index; | 8377 | PTDEBUG && _d('Delete trigger new index:', Dumper($index)); |
21 | 8378 | last; | 8378 | $new_tbl->{del_index} = $index; |
22 | 8379 | } | 8379 | last; |
23 | 8380 | } | 8380 | } |
24 | 8381 | } | ||
25 | 8382 | PTDEBUG && _d('New table delete index:', $new_tbl->{del_index}); | ||
26 | 8383 | } | ||
27 | 8384 | |||
28 | 8385 | { | ||
29 | 8386 | my $indexes = $orig_tbl->{tbl_struct}->{keys}; # brevity | ||
30 | 8387 | foreach my $index ( $tp->sort_indexes($orig_tbl->{tbl_struct}) ) { | ||
31 | 8388 | if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) { | ||
32 | 8389 | PTDEBUG && _d('Delete trigger orig index:', Dumper($index)); | ||
33 | 8390 | $orig_tbl->{del_index} = $index; | ||
34 | 8391 | last; | ||
35 | 8392 | } | ||
36 | 8393 | } | ||
37 | 8394 | PTDEBUG && _d('Orig table delete index:', $orig_tbl->{del_index}); | ||
38 | 8395 | } | ||
39 | 8396 | |||
40 | 8381 | if ( !$new_tbl->{del_index} ) { | 8397 | if ( !$new_tbl->{del_index} ) { |
41 | 8382 | die "The new table $new_tbl->{name} does not have a PRIMARY KEY " | 8398 | die "The new table $new_tbl->{name} does not have a PRIMARY KEY " |
42 | 8383 | . "or a unique index which is required for the DELETE trigger.\n"; | 8399 | . "or a unique index which is required for the DELETE trigger.\n"; |
43 | 8384 | } | 8400 | } |
44 | 8385 | 8401 | ||
45 | 8402 | # Determine whether to use the new or orig table delete index. | ||
46 | 8403 | # The new table del index is preferred due to | ||
47 | 8404 | # https://bugs.launchpad.net/percona-toolkit/+bug/1062324 | ||
48 | 8405 | # In short, if the chosen del index is re-created with new columns, | ||
49 | 8406 | # its original columns may be dropped, so just use its new columns. | ||
50 | 8407 | # But, due to https://bugs.launchpad.net/percona-toolkit/+bug/1103672, | ||
51 | 8408 | # the chosen del index on the new table may reference columns which | ||
52 | 8409 | # do not/no longer exist in the orig table, so we check for this | ||
53 | 8410 | # and, if it's the case, we fall back to using the del index from | ||
54 | 8411 | # the orig table. | ||
55 | 8412 | my $del_tbl = $new_tbl; # preferred | ||
56 | 8413 | my $new_del_index_cols # brevity | ||
57 | 8414 | = $new_tbl->{tbl_struct}->{keys}->{ $new_tbl->{del_index} }->{cols}; | ||
58 | 8415 | foreach my $new_del_index_col ( @$new_del_index_cols ) { | ||
59 | 8416 | if ( !exists $orig_cols->{$new_del_index_col} ) { | ||
60 | 8417 | if ( !$orig_tbl->{del_index} ) { | ||
61 | 8418 | die "The new table index $new_tbl->{del_index} would be used " | ||
62 | 8419 | . "for the DELETE trigger, but it uses column " | ||
63 | 8420 | . "$new_del_index_col which does not exist in the original " | ||
64 | 8421 | . "table and the original table does not have a PRIMARY KEY " | ||
65 | 8422 | . "or a unique index to use for the DELETE trigger.\n"; | ||
66 | 8423 | } | ||
67 | 8424 | print "Using original table index $orig_tbl->{del_index} for the " | ||
68 | 8425 | . "DELETE trigger instead of new table index $new_tbl->{del_index} " | ||
69 | 8426 | . "because the new table index uses column $new_del_index_col " | ||
70 | 8427 | . "which does not exist in the original table.\n"; | ||
71 | 8428 | $del_tbl = $orig_tbl; | ||
72 | 8429 | last; | ||
73 | 8430 | } | ||
74 | 8431 | } | ||
75 | 8432 | |||
76 | 8433 | { | ||
77 | 8434 | my $del_cols | ||
78 | 8435 | = $del_tbl->{tbl_struct}->{keys}->{ $del_tbl->{del_index} }->{cols}; | ||
79 | 8436 | PTDEBUG && _d('Index for delete trigger: table', $del_tbl->{name}, | ||
80 | 8437 | 'index', $del_tbl->{del_index}, | ||
81 | 8438 | 'columns', @$del_cols); | ||
82 | 8439 | } | ||
83 | 8440 | |||
84 | 8386 | # ######################################################################## | 8441 | # ######################################################################## |
85 | 8387 | # Step 3: Create the triggers to capture changes on the original table and | 8442 | # Step 3: Create the triggers to capture changes on the original table and |
86 | 8388 | # apply them to the new table. | 8443 | # apply them to the new table. |
87 | @@ -8412,6 +8467,7 @@ | |||
88 | 8412 | create_triggers( | 8467 | create_triggers( |
89 | 8413 | orig_tbl => $orig_tbl, | 8468 | orig_tbl => $orig_tbl, |
90 | 8414 | new_tbl => $new_tbl, | 8469 | new_tbl => $new_tbl, |
91 | 8470 | del_tbl => $del_tbl, | ||
92 | 8415 | columns => \@common_cols, | 8471 | columns => \@common_cols, |
93 | 8416 | Cxn => $cxn, | 8472 | Cxn => $cxn, |
94 | 8417 | Quoter => $q, | 8473 | Quoter => $q, |
95 | @@ -8931,6 +8987,28 @@ | |||
96 | 8931 | my $ok = 1; | 8987 | my $ok = 1; |
97 | 8932 | 8988 | ||
98 | 8933 | # ######################################################################## | 8989 | # ######################################################################## |
99 | 8990 | # Check for DROP PRIMARY KEY. | ||
100 | 8991 | # ######################################################################## | ||
101 | 8992 | if ( $alter =~ m/DROP\s+PRIMARY\s+KEY/i ) { | ||
102 | 8993 | my $msg = "--alter contains 'DROP PRIMARY KEY'. Dropping and " | ||
103 | 8994 | . "altering the primary key can be dangerous, " | ||
104 | 8995 | . "especially if the original table does not have other " | ||
105 | 8996 | . "unique indexes.\n"; | ||
106 | 8997 | if ( $dry_run ) { | ||
107 | 8998 | print $msg; | ||
108 | 8999 | } | ||
109 | 9000 | else { | ||
110 | 9001 | $ok = 0; | ||
111 | 9002 | warn $msg | ||
112 | 9003 | . "The tool should handle this correctly, but you should " | ||
113 | 9004 | . "test it first and carefully examine the triggers which " | ||
114 | 9005 | . "rely on the PRIMARY KEY or a unique index. Specify " | ||
115 | 9006 | . "--no-check-alter to disable this check and perform the " | ||
116 | 9007 | . "--alter.\n"; | ||
117 | 9008 | } | ||
118 | 9009 | } | ||
119 | 9010 | |||
120 | 9011 | # ######################################################################## | ||
121 | 8934 | # Check for renamed columns. | 9012 | # Check for renamed columns. |
122 | 8935 | # https://bugs.launchpad.net/percona-toolkit/+bug/1068562 | 9013 | # https://bugs.launchpad.net/percona-toolkit/+bug/1068562 |
123 | 8936 | # ######################################################################## | 9014 | # ######################################################################## |
124 | @@ -8976,6 +9054,7 @@ | |||
125 | 8976 | } | 9054 | } |
126 | 8977 | 9055 | ||
127 | 8978 | if ( !$ok ) { | 9056 | if ( !$ok ) { |
128 | 9057 | # check_alter.t relies on this output. | ||
129 | 8979 | die "--check-alter failed.\n"; | 9058 | die "--check-alter failed.\n"; |
130 | 8980 | } | 9059 | } |
131 | 8981 | 9060 | ||
132 | @@ -9536,11 +9615,11 @@ | |||
133 | 9536 | 9615 | ||
134 | 9537 | sub create_triggers { | 9616 | sub create_triggers { |
135 | 9538 | my ( %args ) = @_; | 9617 | my ( %args ) = @_; |
137 | 9539 | my @required_args = qw(orig_tbl new_tbl columns Cxn Quoter OptionParser); | 9618 | my @required_args = qw(orig_tbl new_tbl del_tbl columns Cxn Quoter OptionParser); |
138 | 9540 | foreach my $arg ( @required_args ) { | 9619 | foreach my $arg ( @required_args ) { |
139 | 9541 | die "I need a $arg argument" unless $args{$arg}; | 9620 | die "I need a $arg argument" unless $args{$arg}; |
140 | 9542 | } | 9621 | } |
142 | 9543 | my ($orig_tbl, $new_tbl, $cols, $cxn, $q, $o) = @args{@required_args}; | 9622 | my ($orig_tbl, $new_tbl, $del_tbl, $cols, $cxn, $q, $o) = @args{@required_args}; |
143 | 9544 | 9623 | ||
144 | 9545 | # This sub works for --dry-run and --execute. With --dry-run it's | 9624 | # This sub works for --dry-run and --execute. With --dry-run it's |
145 | 9546 | # only interesting if --print is specified, too; then the user can | 9625 | # only interesting if --print is specified, too; then the user can |
146 | @@ -9569,8 +9648,8 @@ | |||
147 | 9569 | # unique indexes can be nullable. Cols are from the new table and | 9648 | # unique indexes can be nullable. Cols are from the new table and |
148 | 9570 | # they may have been renamed | 9649 | # they may have been renamed |
149 | 9571 | my %old_col_for = map { $_->{new} => $_->{old} } @$cols; | 9650 | my %old_col_for = map { $_->{new} => $_->{old} } @$cols; |
152 | 9572 | my $tbl_struct = $new_tbl->{tbl_struct}; | 9651 | my $tbl_struct = $del_tbl->{tbl_struct}; |
153 | 9573 | my $del_index = $new_tbl->{del_index}; | 9652 | my $del_index = $del_tbl->{del_index}; |
154 | 9574 | my $del_index_cols = join(" AND ", map { | 9653 | my $del_index_cols = join(" AND ", map { |
155 | 9575 | my $new_col = $_; | 9654 | my $new_col = $_; |
156 | 9576 | my $old_col = $old_col_for{$new_col} || $new_col; | 9655 | my $old_col = $old_col_for{$new_col} || $new_col; |
157 | @@ -10226,9 +10305,19 @@ | |||
158 | 10226 | C<CHANGE COLUMN name new_name> would lead to that column's data being lost. | 10305 | C<CHANGE COLUMN name new_name> would lead to that column's data being lost. |
159 | 10227 | The tool now parses the alter statement and tries to catch these cases, so | 10306 | The tool now parses the alter statement and tries to catch these cases, so |
160 | 10228 | the renamed columns should have the same data as the originals. However, the | 10307 | the renamed columns should have the same data as the originals. However, the |
164 | 10229 | code that does this is not a full-blown SQL parser, so we recommend that users | 10308 | code that does this is not a full-blown SQL parser, so you should first |
165 | 10230 | run the tool with L<"--dry-run"> and check if it's detecting the renames | 10309 | run the tool with L<"--dry-run"> and L<"--print"> and verify that it detects |
166 | 10231 | correctly. | 10310 | the renamed columns correctly. |
167 | 10311 | |||
168 | 10312 | =item DROP PRIMARY KEY | ||
169 | 10313 | |||
170 | 10314 | If L<"--alter"> contain C<DROP PRIMARY KEY> (case- and space-insensitive), | ||
171 | 10315 | a warning is printed and the tool exits unless L<"--dry-run"> is specified. | ||
172 | 10316 | Altering the primary key can be dangerous, but the tool can handle it. | ||
173 | 10317 | The tool's triggers, particularly the DELETE trigger, are most affected by | ||
174 | 10318 | altering the primary key because the tool prefers to use the primary key | ||
175 | 10319 | for its triggers. You should first run the tool with L<"--dry-run"> and | ||
176 | 10320 | L<"--print"> and verify that the triggers are correct. | ||
177 | 10232 | 10321 | ||
178 | 10233 | =back | 10322 | =back |
179 | 10234 | 10323 | ||
180 | 10235 | 10324 | ||
181 | === modified file 't/pt-online-schema-change/bugs.t' | |||
182 | --- t/pt-online-schema-change/bugs.t 2012-12-11 16:54:24 +0000 | |||
183 | +++ t/pt-online-schema-change/bugs.t 2013-01-30 15:06:22 +0000 | |||
184 | @@ -210,7 +210,7 @@ | |||
185 | 210 | sub { pt_online_schema_change::main(@args, | 210 | sub { pt_online_schema_change::main(@args, |
186 | 211 | "$master_dsn,D=test,t=t1", | 211 | "$master_dsn,D=test,t=t1", |
187 | 212 | "--alter", "drop key 2bpk, drop key c3, drop primary key, drop c1, add primary key (c2, c3(4)), add key (c3(4))", | 212 | "--alter", "drop key 2bpk, drop key c3, drop primary key, drop c1, add primary key (c2, c3(4)), add key (c3(4))", |
189 | 213 | qw(--execute --no-drop-new-table --no-swap-tables)) }, | 213 | qw(--no-check-alter --execute --no-drop-new-table --no-swap-tables)) }, |
190 | 214 | ); | 214 | ); |
191 | 215 | 215 | ||
192 | 216 | # Since _t1_new no longer has the c1 column, the bug caused this | 216 | # Since _t1_new no longer has the c1 column, the bug caused this |
193 | @@ -233,6 +233,34 @@ | |||
194 | 233 | undef, | 233 | undef, |
195 | 234 | "Delete trigger works after altering PK (bug 1062324)" | 234 | "Delete trigger works after altering PK (bug 1062324)" |
196 | 235 | ); | 235 | ); |
197 | 236 | |||
198 | 237 | # Another instance of this bug: | ||
199 | 238 | # https://bugs.launchpad.net/percona-toolkit/+bug/1103672 | ||
200 | 239 | $sb->load_file('master', "$sample/del-trg-bug-1103672.sql"); | ||
201 | 240 | |||
202 | 241 | ($output, $exit_status) = full_output( | ||
203 | 242 | sub { pt_online_schema_change::main(@args, | ||
204 | 243 | "$master_dsn,D=test,t=t1", | ||
205 | 244 | "--alter", "drop primary key, add column _id int unsigned not null primary key auto_increment FIRST", | ||
206 | 245 | qw(--no-check-alter --execute --no-drop-new-table --no-swap-tables)) }, | ||
207 | 246 | ); | ||
208 | 247 | |||
209 | 248 | eval { | ||
210 | 249 | $master_dbh->do("DELETE FROM test.t1 WHERE id=1"); | ||
211 | 250 | }; | ||
212 | 251 | is( | ||
213 | 252 | $EVAL_ERROR, | ||
214 | 253 | "", | ||
215 | 254 | "No delete trigger error after altering PK (bug 1103672)" | ||
216 | 255 | ) or diag($output); | ||
217 | 256 | |||
218 | 257 | $row = $master_dbh->selectrow_arrayref("SELECT * FROM test._t1_new WHERE id=1"); | ||
219 | 258 | is( | ||
220 | 259 | $row, | ||
221 | 260 | undef, | ||
222 | 261 | "Delete trigger works after altering PK (bug 1103672)" | ||
223 | 262 | ); | ||
224 | 263 | |||
225 | 236 | } | 264 | } |
226 | 237 | 265 | ||
227 | 238 | # ############################################################################# | 266 | # ############################################################################# |
228 | 239 | 267 | ||
229 | === added file 't/pt-online-schema-change/check_alter.t' | |||
230 | --- t/pt-online-schema-change/check_alter.t 1970-01-01 00:00:00 +0000 | |||
231 | +++ t/pt-online-schema-change/check_alter.t 2013-01-30 15:06:22 +0000 | |||
232 | @@ -0,0 +1,65 @@ | |||
233 | 1 | #!/usr/bin/env perl | ||
234 | 2 | |||
235 | 3 | BEGIN { | ||
236 | 4 | die "The PERCONA_TOOLKIT_BRANCH environment variable is not set.\n" | ||
237 | 5 | unless $ENV{PERCONA_TOOLKIT_BRANCH} && -d $ENV{PERCONA_TOOLKIT_BRANCH}; | ||
238 | 6 | unshift @INC, "$ENV{PERCONA_TOOLKIT_BRANCH}/lib"; | ||
239 | 7 | }; | ||
240 | 8 | |||
241 | 9 | use strict; | ||
242 | 10 | use warnings FATAL => 'all'; | ||
243 | 11 | use English qw(-no_match_vars); | ||
244 | 12 | use Test::More; | ||
245 | 13 | |||
246 | 14 | use Data::Dumper; | ||
247 | 15 | use PerconaTest; | ||
248 | 16 | use Sandbox; | ||
249 | 17 | |||
250 | 18 | require "$trunk/bin/pt-online-schema-change"; | ||
251 | 19 | |||
252 | 20 | my $dp = new DSNParser(opts=>$dsn_opts); | ||
253 | 21 | my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp); | ||
254 | 22 | my $master_dbh = $sb->get_dbh_for('master'); | ||
255 | 23 | my $slave_dbh = $sb->get_dbh_for('slave1'); | ||
256 | 24 | |||
257 | 25 | if ( !$master_dbh ) { | ||
258 | 26 | plan skip_all => 'Cannot connect to sandbox master'; | ||
259 | 27 | } | ||
260 | 28 | elsif ( !$slave_dbh ) { | ||
261 | 29 | plan skip_all => 'Cannot connect to sandbox slave1'; | ||
262 | 30 | } | ||
263 | 31 | |||
264 | 32 | # The sandbox servers run with lock_wait_timeout=3 and it's not dynamic | ||
265 | 33 | # so we need to specify --lock-wait-timeout=3 else the tool will die. | ||
266 | 34 | my $master_dsn = 'h=127.1,P=12345,u=msandbox,p=msandbox'; | ||
267 | 35 | my @args = (qw(--lock-wait-timeout 3)); | ||
268 | 36 | my $output; | ||
269 | 37 | my $exit_status; | ||
270 | 38 | my $sample = "t/pt-online-schema-change/samples/"; | ||
271 | 39 | |||
272 | 40 | # ############################################################################# | ||
273 | 41 | # DROP PRIMARY KEY | ||
274 | 42 | # ############################################################################# | ||
275 | 43 | |||
276 | 44 | $sb->load_file('master', "$sample/del-trg-bug-1103672.sql"); | ||
277 | 45 | |||
278 | 46 | ($output, $exit_status) = full_output( | ||
279 | 47 | sub { pt_online_schema_change::main(@args, | ||
280 | 48 | "$master_dsn,D=test,t=t1", | ||
281 | 49 | "--alter", "drop primary key, add column _id int unsigned not null primary key auto_increment FIRST", | ||
282 | 50 | qw(--execute)), | ||
283 | 51 | }, | ||
284 | 52 | ); | ||
285 | 53 | |||
286 | 54 | like( | ||
287 | 55 | $output, | ||
288 | 56 | qr/--check-alter failed/, | ||
289 | 57 | "DROP PRIMARY KEY" | ||
290 | 58 | ); | ||
291 | 59 | |||
292 | 60 | # ############################################################################# | ||
293 | 61 | # Done. | ||
294 | 62 | # ############################################################################# | ||
295 | 63 | $sb->wipe_clean($master_dbh); | ||
296 | 64 | ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox"); | ||
297 | 65 | done_testing; | ||
298 | 0 | 66 | ||
299 | === added file 't/pt-online-schema-change/samples/del-trg-bug-1103672.sql' | |||
300 | --- t/pt-online-schema-change/samples/del-trg-bug-1103672.sql 1970-01-01 00:00:00 +0000 | |||
301 | +++ t/pt-online-schema-change/samples/del-trg-bug-1103672.sql 2013-01-30 15:06:22 +0000 | |||
302 | @@ -0,0 +1,20 @@ | |||
303 | 1 | drop database if exists test; | ||
304 | 2 | create database test; | ||
305 | 3 | use test; | ||
306 | 4 | |||
307 | 5 | CREATE TABLE `t1` ( | ||
308 | 6 | `id` int(10) unsigned NOT NULL, | ||
309 | 7 | `x` char(3) DEFAULT NULL, | ||
310 | 8 | PRIMARY KEY (`id`) | ||
311 | 9 | ) ENGINE=InnoDB; | ||
312 | 10 | |||
313 | 11 | INSERT INTO t1 VALUES | ||
314 | 12 | (1, 'a'), | ||
315 | 13 | (2, 'b'), | ||
316 | 14 | (3, 'c'), | ||
317 | 15 | (4, 'd'), | ||
318 | 16 | (5, 'f'), | ||
319 | 17 | (6, 'g'), | ||
320 | 18 | (7, 'h'), | ||
321 | 19 | (8, 'i'), | ||
322 | 20 | (9, 'j'); |