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:
|
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
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 | # Find a pk or unique index to use for the delete trigger. can_nibble() |
6 | # above returns an index, but NibbleIterator will use non-unique indexes, |
7 | # so we have to do this again here. |
8 | - my $indexes = $new_tbl->{tbl_struct}->{keys}; # brevity |
9 | - foreach my $index ( $tp->sort_indexes($new_tbl->{tbl_struct}) ) { |
10 | - if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) { |
11 | - PTDEBUG && _d('Delete trigger index:', Dumper($index)); |
12 | - $new_tbl->{del_index} = $index; |
13 | - last; |
14 | - } |
15 | - } |
16 | + { |
17 | + my $indexes = $new_tbl->{tbl_struct}->{keys}; # brevity |
18 | + foreach my $index ( $tp->sort_indexes($new_tbl->{tbl_struct}) ) { |
19 | + if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) { |
20 | + PTDEBUG && _d('Delete trigger new index:', Dumper($index)); |
21 | + $new_tbl->{del_index} = $index; |
22 | + last; |
23 | + } |
24 | + } |
25 | + PTDEBUG && _d('New table delete index:', $new_tbl->{del_index}); |
26 | + } |
27 | + |
28 | + { |
29 | + my $indexes = $orig_tbl->{tbl_struct}->{keys}; # brevity |
30 | + foreach my $index ( $tp->sort_indexes($orig_tbl->{tbl_struct}) ) { |
31 | + if ( $index eq 'PRIMARY' || $indexes->{$index}->{is_unique} ) { |
32 | + PTDEBUG && _d('Delete trigger orig index:', Dumper($index)); |
33 | + $orig_tbl->{del_index} = $index; |
34 | + last; |
35 | + } |
36 | + } |
37 | + PTDEBUG && _d('Orig table delete index:', $orig_tbl->{del_index}); |
38 | + } |
39 | + |
40 | if ( !$new_tbl->{del_index} ) { |
41 | die "The new table $new_tbl->{name} does not have a PRIMARY KEY " |
42 | . "or a unique index which is required for the DELETE trigger.\n"; |
43 | } |
44 | |
45 | + # Determine whether to use the new or orig table delete index. |
46 | + # The new table del index is preferred due to |
47 | + # https://bugs.launchpad.net/percona-toolkit/+bug/1062324 |
48 | + # In short, if the chosen del index is re-created with new columns, |
49 | + # its original columns may be dropped, so just use its new columns. |
50 | + # But, due to https://bugs.launchpad.net/percona-toolkit/+bug/1103672, |
51 | + # the chosen del index on the new table may reference columns which |
52 | + # do not/no longer exist in the orig table, so we check for this |
53 | + # and, if it's the case, we fall back to using the del index from |
54 | + # the orig table. |
55 | + my $del_tbl = $new_tbl; # preferred |
56 | + my $new_del_index_cols # brevity |
57 | + = $new_tbl->{tbl_struct}->{keys}->{ $new_tbl->{del_index} }->{cols}; |
58 | + foreach my $new_del_index_col ( @$new_del_index_cols ) { |
59 | + if ( !exists $orig_cols->{$new_del_index_col} ) { |
60 | + if ( !$orig_tbl->{del_index} ) { |
61 | + die "The new table index $new_tbl->{del_index} would be used " |
62 | + . "for the DELETE trigger, but it uses column " |
63 | + . "$new_del_index_col which does not exist in the original " |
64 | + . "table and the original table does not have a PRIMARY KEY " |
65 | + . "or a unique index to use for the DELETE trigger.\n"; |
66 | + } |
67 | + print "Using original table index $orig_tbl->{del_index} for the " |
68 | + . "DELETE trigger instead of new table index $new_tbl->{del_index} " |
69 | + . "because the new table index uses column $new_del_index_col " |
70 | + . "which does not exist in the original table.\n"; |
71 | + $del_tbl = $orig_tbl; |
72 | + last; |
73 | + } |
74 | + } |
75 | + |
76 | + { |
77 | + my $del_cols |
78 | + = $del_tbl->{tbl_struct}->{keys}->{ $del_tbl->{del_index} }->{cols}; |
79 | + PTDEBUG && _d('Index for delete trigger: table', $del_tbl->{name}, |
80 | + 'index', $del_tbl->{del_index}, |
81 | + 'columns', @$del_cols); |
82 | + } |
83 | + |
84 | # ######################################################################## |
85 | # Step 3: Create the triggers to capture changes on the original table and |
86 | # apply them to the new table. |
87 | @@ -8412,6 +8467,7 @@ |
88 | create_triggers( |
89 | orig_tbl => $orig_tbl, |
90 | new_tbl => $new_tbl, |
91 | + del_tbl => $del_tbl, |
92 | columns => \@common_cols, |
93 | Cxn => $cxn, |
94 | Quoter => $q, |
95 | @@ -8931,6 +8987,28 @@ |
96 | my $ok = 1; |
97 | |
98 | # ######################################################################## |
99 | + # Check for DROP PRIMARY KEY. |
100 | + # ######################################################################## |
101 | + if ( $alter =~ m/DROP\s+PRIMARY\s+KEY/i ) { |
102 | + my $msg = "--alter contains 'DROP PRIMARY KEY'. Dropping and " |
103 | + . "altering the primary key can be dangerous, " |
104 | + . "especially if the original table does not have other " |
105 | + . "unique indexes.\n"; |
106 | + if ( $dry_run ) { |
107 | + print $msg; |
108 | + } |
109 | + else { |
110 | + $ok = 0; |
111 | + warn $msg |
112 | + . "The tool should handle this correctly, but you should " |
113 | + . "test it first and carefully examine the triggers which " |
114 | + . "rely on the PRIMARY KEY or a unique index. Specify " |
115 | + . "--no-check-alter to disable this check and perform the " |
116 | + . "--alter.\n"; |
117 | + } |
118 | + } |
119 | + |
120 | + # ######################################################################## |
121 | # Check for renamed columns. |
122 | # https://bugs.launchpad.net/percona-toolkit/+bug/1068562 |
123 | # ######################################################################## |
124 | @@ -8976,6 +9054,7 @@ |
125 | } |
126 | |
127 | if ( !$ok ) { |
128 | + # check_alter.t relies on this output. |
129 | die "--check-alter failed.\n"; |
130 | } |
131 | |
132 | @@ -9536,11 +9615,11 @@ |
133 | |
134 | sub create_triggers { |
135 | my ( %args ) = @_; |
136 | - my @required_args = qw(orig_tbl new_tbl columns Cxn Quoter OptionParser); |
137 | + my @required_args = qw(orig_tbl new_tbl del_tbl columns Cxn Quoter OptionParser); |
138 | foreach my $arg ( @required_args ) { |
139 | die "I need a $arg argument" unless $args{$arg}; |
140 | } |
141 | - my ($orig_tbl, $new_tbl, $cols, $cxn, $q, $o) = @args{@required_args}; |
142 | + my ($orig_tbl, $new_tbl, $del_tbl, $cols, $cxn, $q, $o) = @args{@required_args}; |
143 | |
144 | # This sub works for --dry-run and --execute. With --dry-run it's |
145 | # only interesting if --print is specified, too; then the user can |
146 | @@ -9569,8 +9648,8 @@ |
147 | # unique indexes can be nullable. Cols are from the new table and |
148 | # they may have been renamed |
149 | my %old_col_for = map { $_->{new} => $_->{old} } @$cols; |
150 | - my $tbl_struct = $new_tbl->{tbl_struct}; |
151 | - my $del_index = $new_tbl->{del_index}; |
152 | + my $tbl_struct = $del_tbl->{tbl_struct}; |
153 | + my $del_index = $del_tbl->{del_index}; |
154 | my $del_index_cols = join(" AND ", map { |
155 | my $new_col = $_; |
156 | my $old_col = $old_col_for{$new_col} || $new_col; |
157 | @@ -10226,9 +10305,19 @@ |
158 | C<CHANGE COLUMN name new_name> would lead to that column's data being lost. |
159 | The tool now parses the alter statement and tries to catch these cases, so |
160 | the renamed columns should have the same data as the originals. However, the |
161 | -code that does this is not a full-blown SQL parser, so we recommend that users |
162 | -run the tool with L<"--dry-run"> and check if it's detecting the renames |
163 | -correctly. |
164 | +code that does this is not a full-blown SQL parser, so you should first |
165 | +run the tool with L<"--dry-run"> and L<"--print"> and verify that it detects |
166 | +the renamed columns correctly. |
167 | + |
168 | +=item DROP PRIMARY KEY |
169 | + |
170 | +If L<"--alter"> contain C<DROP PRIMARY KEY> (case- and space-insensitive), |
171 | +a warning is printed and the tool exits unless L<"--dry-run"> is specified. |
172 | +Altering the primary key can be dangerous, but the tool can handle it. |
173 | +The tool's triggers, particularly the DELETE trigger, are most affected by |
174 | +altering the primary key because the tool prefers to use the primary key |
175 | +for its triggers. You should first run the tool with L<"--dry-run"> and |
176 | +L<"--print"> and verify that the triggers are correct. |
177 | |
178 | =back |
179 | |
180 | |
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 | sub { pt_online_schema_change::main(@args, |
186 | "$master_dsn,D=test,t=t1", |
187 | "--alter", "drop key 2bpk, drop key c3, drop primary key, drop c1, add primary key (c2, c3(4)), add key (c3(4))", |
188 | - qw(--execute --no-drop-new-table --no-swap-tables)) }, |
189 | + qw(--no-check-alter --execute --no-drop-new-table --no-swap-tables)) }, |
190 | ); |
191 | |
192 | # Since _t1_new no longer has the c1 column, the bug caused this |
193 | @@ -233,6 +233,34 @@ |
194 | undef, |
195 | "Delete trigger works after altering PK (bug 1062324)" |
196 | ); |
197 | + |
198 | + # Another instance of this bug: |
199 | + # https://bugs.launchpad.net/percona-toolkit/+bug/1103672 |
200 | + $sb->load_file('master', "$sample/del-trg-bug-1103672.sql"); |
201 | + |
202 | + ($output, $exit_status) = full_output( |
203 | + sub { pt_online_schema_change::main(@args, |
204 | + "$master_dsn,D=test,t=t1", |
205 | + "--alter", "drop primary key, add column _id int unsigned not null primary key auto_increment FIRST", |
206 | + qw(--no-check-alter --execute --no-drop-new-table --no-swap-tables)) }, |
207 | + ); |
208 | + |
209 | + eval { |
210 | + $master_dbh->do("DELETE FROM test.t1 WHERE id=1"); |
211 | + }; |
212 | + is( |
213 | + $EVAL_ERROR, |
214 | + "", |
215 | + "No delete trigger error after altering PK (bug 1103672)" |
216 | + ) or diag($output); |
217 | + |
218 | + $row = $master_dbh->selectrow_arrayref("SELECT * FROM test._t1_new WHERE id=1"); |
219 | + is( |
220 | + $row, |
221 | + undef, |
222 | + "Delete trigger works after altering PK (bug 1103672)" |
223 | + ); |
224 | + |
225 | } |
226 | |
227 | # ############################################################################# |
228 | |
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 | +#!/usr/bin/env perl |
234 | + |
235 | +BEGIN { |
236 | + die "The PERCONA_TOOLKIT_BRANCH environment variable is not set.\n" |
237 | + unless $ENV{PERCONA_TOOLKIT_BRANCH} && -d $ENV{PERCONA_TOOLKIT_BRANCH}; |
238 | + unshift @INC, "$ENV{PERCONA_TOOLKIT_BRANCH}/lib"; |
239 | +}; |
240 | + |
241 | +use strict; |
242 | +use warnings FATAL => 'all'; |
243 | +use English qw(-no_match_vars); |
244 | +use Test::More; |
245 | + |
246 | +use Data::Dumper; |
247 | +use PerconaTest; |
248 | +use Sandbox; |
249 | + |
250 | +require "$trunk/bin/pt-online-schema-change"; |
251 | + |
252 | +my $dp = new DSNParser(opts=>$dsn_opts); |
253 | +my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp); |
254 | +my $master_dbh = $sb->get_dbh_for('master'); |
255 | +my $slave_dbh = $sb->get_dbh_for('slave1'); |
256 | + |
257 | +if ( !$master_dbh ) { |
258 | + plan skip_all => 'Cannot connect to sandbox master'; |
259 | +} |
260 | +elsif ( !$slave_dbh ) { |
261 | + plan skip_all => 'Cannot connect to sandbox slave1'; |
262 | +} |
263 | + |
264 | +# The sandbox servers run with lock_wait_timeout=3 and it's not dynamic |
265 | +# so we need to specify --lock-wait-timeout=3 else the tool will die. |
266 | +my $master_dsn = 'h=127.1,P=12345,u=msandbox,p=msandbox'; |
267 | +my @args = (qw(--lock-wait-timeout 3)); |
268 | +my $output; |
269 | +my $exit_status; |
270 | +my $sample = "t/pt-online-schema-change/samples/"; |
271 | + |
272 | +# ############################################################################# |
273 | +# DROP PRIMARY KEY |
274 | +# ############################################################################# |
275 | + |
276 | +$sb->load_file('master', "$sample/del-trg-bug-1103672.sql"); |
277 | + |
278 | +($output, $exit_status) = full_output( |
279 | + sub { pt_online_schema_change::main(@args, |
280 | + "$master_dsn,D=test,t=t1", |
281 | + "--alter", "drop primary key, add column _id int unsigned not null primary key auto_increment FIRST", |
282 | + qw(--execute)), |
283 | + }, |
284 | +); |
285 | + |
286 | +like( |
287 | + $output, |
288 | + qr/--check-alter failed/, |
289 | + "DROP PRIMARY KEY" |
290 | +); |
291 | + |
292 | +# ############################################################################# |
293 | +# Done. |
294 | +# ############################################################################# |
295 | +$sb->wipe_clean($master_dbh); |
296 | +ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox"); |
297 | +done_testing; |
298 | |
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 | +drop database if exists test; |
304 | +create database test; |
305 | +use test; |
306 | + |
307 | +CREATE TABLE `t1` ( |
308 | + `id` int(10) unsigned NOT NULL, |
309 | + `x` char(3) DEFAULT NULL, |
310 | + PRIMARY KEY (`id`) |
311 | +) ENGINE=InnoDB; |
312 | + |
313 | +INSERT INTO t1 VALUES |
314 | + (1, 'a'), |
315 | + (2, 'b'), |
316 | + (3, 'c'), |
317 | + (4, 'd'), |
318 | + (5, 'f'), |
319 | + (6, 'g'), |
320 | + (7, 'h'), |
321 | + (8, 'i'), |
322 | + (9, 'j'); |