Merge lp:~percona-toolkit-dev/percona-toolkit/pk-del-bug-1103672-encore into lp:percona-toolkit/2.1

Proposed by Daniel Nichter on 2013-01-30
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
Reviewer Review Type Date Requested Status
Daniel Nichter Approve on 2013-01-30
Review via email: mp+145527@code.launchpad.net
To post a comment you must log in.
review: Approve
523. By Daniel Nichter on 2013-01-30

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');

Subscribers

People subscribed via source and target branches