Merge lp:~percona-toolkit-dev/percona-toolkit/fix-osc-repl-bug-933232 into lp:percona-toolkit/2.0
- fix-osc-repl-bug-933232
- Merge into 2.0
Proposed by
Daniel Nichter
Status: | Superseded | ||||
---|---|---|---|---|---|
Proposed branch: | lp:~percona-toolkit-dev/percona-toolkit/fix-osc-repl-bug-933232 | ||||
Merge into: | lp:percona-toolkit/2.0 | ||||
Diff against target: |
349 lines (+111/-34) (has conflicts) 4 files modified
bin/pt-online-schema-change (+39/-9) t/pt-online-schema-change/alter_active_table.t (+1/-1) t/pt-online-schema-change/basics.t (+63/-23) t/pt-online-schema-change/option_sanity.t (+8/-1) Text conflict in t/pt-online-schema-change/basics.t |
||||
To merge this branch: | bzr merge lp:~percona-toolkit-dev/percona-toolkit/fix-osc-repl-bug-933232 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Daniel Nichter | Approve | ||
Review via email: mp+95660@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
- 188. By Daniel Nichter
-
Add --execute and die unless it's given. Enhance docu/risks about replication.
Unmerged revisions
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-02-21 16:06:08 +0000 |
3 | +++ bin/pt-online-schema-change 2012-03-02 20:33:18 +0000 |
4 | @@ -4110,6 +4110,13 @@ |
5 | # ##################################################################### |
6 | # Do the online alter. |
7 | # ##################################################################### |
8 | + if ( !$o->get('execute') ) { |
9 | + msg("Exiting without altering $db.$tbl because you did not " |
10 | + . "specify --execute. Please read the tool's documentation " |
11 | + . "carefully before using this tool."); |
12 | + return $exit_status; |
13 | + } |
14 | + |
15 | msg("Starting online schema change"); |
16 | eval { |
17 | my $sql = ""; |
18 | @@ -4218,7 +4225,7 @@ |
19 | msg($sql); |
20 | $dbh->do($sql) unless $o->get('print'); |
21 | |
22 | - $sql = "DROP TABLE `$db`.`$tbl`"; |
23 | + $sql = "DROP TABLE IF EXISTS `$db`.`$tbl`"; |
24 | msg($sql); |
25 | $dbh->do($sql) unless $o->get('print'); |
26 | |
27 | @@ -4241,7 +4248,7 @@ |
28 | $capture_sync->cleanup(%plugin_args); |
29 | |
30 | if ( $o->get('rename-tables') && $o->get('drop-old-table') ) { |
31 | - $sql = "DROP TABLE `$db`.`$old_tbl`"; |
32 | + $sql = "DROP TABLE IF EXISTS `$db`.`$old_tbl`"; |
33 | msg($sql); |
34 | $dbh->do($sql) unless $o->get('print'); |
35 | } |
36 | @@ -4510,12 +4517,12 @@ |
37 | are those created by the nature of the tool (e.g. read-only tools vs. read-write |
38 | tools) and those created by bugs. |
39 | |
40 | -pt-online-schema-change reads, writes, alters and drops tables. Although |
41 | -it is tested, do not use it in production until you have thoroughly tested it |
42 | -in your environment! |
43 | +pt-online-schema-change alters tables and data, therefore it is a high-risk |
44 | +tool. Although the tool is tested and is known to work, you should not use |
45 | +it in production until you have thoroughly tested it in your environment! |
46 | |
47 | -This tool has not been tested with replication; it may break replication. |
48 | -See L<"REPLICATION">. |
49 | +This tool has not been tested with replication, and it can break replication |
50 | +if not used properly. See L<"REPLICATION">. |
51 | |
52 | At the time of this release there are no known bugs that pose a serious risk. |
53 | |
54 | @@ -4604,8 +4611,24 @@ |
55 | |
56 | =head1 REPLICATION |
57 | |
58 | -In brief: update slaves first if columns are added or removed. Certain |
59 | -ALTER changes like ENGINE may not affect replication. |
60 | +If you use pt-online-schema-change to alter a table on a server with slaves, |
61 | +be aware that: |
62 | + |
63 | +=over |
64 | + |
65 | +=item * The tool is not tested with replication |
66 | + |
67 | +=item * The tool can break replication if not used properly |
68 | + |
69 | +=item * Although the tool sets SQL_BIN_LOG=0 by default (unless L<"--bin-log"> is specified), triggers which track changes to the table being altered still write statements to the binary log |
70 | + |
71 | +=item * Replicaiton will break if you alter a table on a master that does not exist on a slave |
72 | + |
73 | +=item * Update slaves first if columns are being added or removed |
74 | + |
75 | +=item * Do not use this tool in production before testing it |
76 | + |
77 | +=back |
78 | |
79 | =head1 OUTPUT |
80 | |
81 | @@ -4737,6 +4760,13 @@ |
82 | If altering a table with foreign key constraints, you may need to specify |
83 | this option depending on which L<"--update-foreign-keys-method"> you choose. |
84 | |
85 | +=item --execute |
86 | + |
87 | +Alter the table. This option must be specified to alter the table, else |
88 | +the tool will only check the table and exit. This helps ensure that the |
89 | +user has read the documentation and is aware of the inherent risks of using |
90 | +this tool. |
91 | + |
92 | =item --[no]foreign-key-checks |
93 | |
94 | default: yes |
95 | |
96 | === modified file 't/pt-online-schema-change/alter_active_table.t' |
97 | --- t/pt-online-schema-change/alter_active_table.t 2011-08-02 21:14:06 +0000 |
98 | +++ t/pt-online-schema-change/alter_active_table.t 2012-03-02 20:33:18 +0000 |
99 | @@ -33,7 +33,7 @@ |
100 | |
101 | my $output = ""; |
102 | my $cnf = '/tmp/12345/my.sandbox.cnf'; |
103 | -my @args = ('-F', $cnf); |
104 | +my @args = ('-F', $cnf, '--execute'); |
105 | my $exit = 0; |
106 | my $rows; |
107 | |
108 | |
109 | === modified file 't/pt-online-schema-change/basics.t' |
110 | --- t/pt-online-schema-change/basics.t 2012-02-21 21:26:44 +0000 |
111 | +++ t/pt-online-schema-change/basics.t 2012-03-02 20:33:18 +0000 |
112 | @@ -19,23 +19,42 @@ |
113 | |
114 | my $dp = new DSNParser(opts=>$dsn_opts); |
115 | my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp); |
116 | -my $dbh = $sb->get_dbh_for('master'); |
117 | +my $master_dbh = $sb->get_dbh_for('master'); |
118 | +my $slave_dbh = $sb->get_dbh_for('slave1'); |
119 | |
120 | -if ( !$dbh ) { |
121 | +if ( !$master_dbh ) { |
122 | plan skip_all => 'Cannot connect to sandbox master'; |
123 | } |
124 | +elsif ( !$slave_dbh ) { |
125 | + plan skip_all => 'Cannot connect to sandbox slave'; |
126 | +} |
127 | else { |
128 | - plan tests => 22; |
129 | + plan tests => 25; |
130 | } |
131 | |
132 | my $output = ""; |
133 | my $cnf = '/tmp/12345/my.sandbox.cnf'; |
134 | -my @args = ('-F', $cnf); |
135 | +my @args = ('-F', $cnf, '--execute'); |
136 | my $exit = 0; |
137 | my $rows; |
138 | |
139 | $sb->load_file('master', "t/pt-online-schema-change/samples/small_table.sql"); |
140 | -$dbh->do('use mkosc'); |
141 | +PerconaTest::wait_for_table($slave_dbh, "mkosc.a", "c='r'"); |
142 | +$master_dbh->do('use mkosc'); |
143 | +$slave_dbh->do('use mkosc'); |
144 | + |
145 | +# ############################################################################# |
146 | +# Tool shouldn't run without --execute (bug 933232). |
147 | +# ############################################################################# |
148 | +$output = output( |
149 | + sub { pt_online_schema_change::main('h=127.1,P=12345,u=msandbox,p=msandbox,D=mkosc,t=a') }, |
150 | +); |
151 | + |
152 | +like( |
153 | + $output, |
154 | + qr/you did not specify --execute/, |
155 | + "Doesn't run without --execute" |
156 | +); |
157 | |
158 | # ############################################################################# |
159 | # --check-tables-and-exit |
160 | @@ -86,7 +105,7 @@ |
161 | 'D=mkosc,t=a', qw(--alter ENGINE=InnoDB)) }, |
162 | ); |
163 | |
164 | -$rows = $dbh->selectall_hashref('show table status from mkosc', 'name'); |
165 | +$rows = $master_dbh->selectall_hashref('show table status from mkosc', 'name'); |
166 | is( |
167 | $rows->{a}->{engine}, |
168 | 'InnoDB', |
169 | @@ -99,8 +118,8 @@ |
170 | "Kept old table, ENGINE=MyISAM" |
171 | ); |
172 | |
173 | -my $org_rows = $dbh->selectall_arrayref('select * from mkosc.__old_a order by i'); |
174 | -my $new_rows = $dbh->selectall_arrayref('select * from mkosc.a order by i'); |
175 | +my $org_rows = $master_dbh->selectall_arrayref('select * from mkosc.__old_a order by i'); |
176 | +my $new_rows = $master_dbh->selectall_arrayref('select * from mkosc.a order by i'); |
177 | is_deeply( |
178 | $new_rows, |
179 | $org_rows, |
180 | @@ -113,10 +132,23 @@ |
181 | "Exit status 0" |
182 | ); |
183 | |
184 | +# Tool should set SQL_LOG_BIN=0 by default, so the table |
185 | +# on the slave should not have changed. |
186 | +$rows = $slave_dbh->selectall_hashref('show table status from mkosc', 'name'); |
187 | +is( |
188 | + $rows->{a}->{engine}, |
189 | + 'MyISAM', |
190 | + "SQL_LOG_BIN=0 by default" |
191 | +); |
192 | + |
193 | # ############################################################################# |
194 | # No --alter and --drop-old-table. |
195 | # ############################################################################# |
196 | +<<<<<<< TREE |
197 | $dbh->do('drop table if exists mkosc.__old_a'); # from previous run |
198 | +======= |
199 | +$master_dbh->do('drop table if exists mkosc.__old_a'); # from prev run |
200 | +>>>>>>> MERGE-SOURCE |
201 | $sb->load_file('master', "t/pt-online-schema-change/samples/small_table.sql"); |
202 | |
203 | output( |
204 | @@ -124,7 +156,7 @@ |
205 | 'D=mkosc,t=a', qw(--drop-old-table)) }, |
206 | ); |
207 | |
208 | -$rows = $dbh->selectall_hashref('show table status from mkosc', 'name'); |
209 | +$rows = $master_dbh->selectall_hashref('show table status from mkosc', 'name'); |
210 | is( |
211 | $rows->{a}->{engine}, |
212 | 'MyISAM', |
213 | @@ -136,7 +168,7 @@ |
214 | "--drop-old-table" |
215 | ); |
216 | |
217 | -$new_rows = $dbh->selectall_arrayref('select * from mkosc.a order by i'); |
218 | +$new_rows = $master_dbh->selectall_arrayref('select * from mkosc.a order by i'); |
219 | is_deeply( |
220 | $new_rows, |
221 | $org_rows, # from previous run since old table was dropped this run |
222 | @@ -149,6 +181,14 @@ |
223 | "Exit status 0" |
224 | ); |
225 | |
226 | +# Bug 933232: drop temp table replicates and break replication. |
227 | +$rows = $slave_dbh->selectrow_hashref('show slave status'); |
228 | +is( |
229 | + $rows->{last_error}, |
230 | + '', |
231 | + "Dropping temp table doesn't break replication (bug 933232)" |
232 | +); |
233 | + |
234 | # ############################################################################ |
235 | # Alter a table with foreign keys. |
236 | # ############################################################################ |
237 | @@ -164,7 +204,7 @@ |
238 | $sb->load_file('master', "t/pt-online-schema-change/samples/fk_tables_schema.sql"); |
239 | |
240 | # city has a fk constraint on country, so get its original table def. |
241 | -my $orig_table_def = $dbh->selectrow_hashref('show create table mkosc.city')->{'create table'}; |
242 | +my $orig_table_def = $master_dbh->selectrow_hashref('show create table mkosc.city')->{'create table'}; |
243 | |
244 | # Alter the parent table. The error we need to avoid is: |
245 | # DBD::mysql::db do failed: Cannot delete or update a parent row: |
246 | @@ -181,7 +221,7 @@ |
247 | "Exit status 0 (rebuild_constraints method)" |
248 | ); |
249 | |
250 | -$rows = $dbh->selectall_arrayref('show tables from mkosc like "\_\_%"'); |
251 | +$rows = $master_dbh->selectall_arrayref('show tables from mkosc like "\_\_%"'); |
252 | is_deeply( |
253 | $rows, |
254 | [], |
255 | @@ -193,7 +233,7 @@ |
256 | # also updated city's fk constraint to __old_country. We should have |
257 | # dropped and re-added that constraint exactly, changing only __old_country |
258 | # to country, like it originally was. |
259 | -my $new_table_def = $dbh->selectrow_hashref('show create table mkosc.city')->{'create table'}; |
260 | +my $new_table_def = $master_dbh->selectrow_hashref('show create table mkosc.city')->{'create table'}; |
261 | is( |
262 | $new_table_def, |
263 | $orig_table_def, |
264 | @@ -205,7 +245,7 @@ |
265 | ####################### |
266 | $sb->load_file('master', "t/pt-online-schema-change/samples/fk_tables_schema.sql"); |
267 | |
268 | -$orig_table_def = $dbh->selectrow_hashref('show create table mkosc.city')->{'create table'}; |
269 | +$orig_table_def = $master_dbh->selectrow_hashref('show create table mkosc.city')->{'create table'}; |
270 | |
271 | output( |
272 | sub { $exit = pt_online_schema_change::main(@args, |
273 | @@ -218,14 +258,14 @@ |
274 | "Exit status 0 (drop_old_table method)" |
275 | ); |
276 | |
277 | -$rows = $dbh->selectall_arrayref('show tables from mkosc like "\_\_%"'); |
278 | +$rows = $master_dbh->selectall_arrayref('show tables from mkosc like "\_\_%"'); |
279 | is_deeply( |
280 | $rows, |
281 | [], |
282 | "Old table dropped (drop_old_table method)" |
283 | ) or print Dumper($rows); |
284 | |
285 | -$new_table_def = $dbh->selectrow_hashref('show create table mkosc.city')->{'create table'}; |
286 | +$new_table_def = $master_dbh->selectrow_hashref('show create table mkosc.city')->{'create table'}; |
287 | is( |
288 | $new_table_def, |
289 | $orig_table_def, |
290 | @@ -240,19 +280,19 @@ |
291 | my ($file, $name) = @args{qw(file name)}; |
292 | |
293 | $sb->load_file('master', "t/lib/samples/osc/$file"); |
294 | - PerconaTest::wait_for_table($dbh, "osc.t", "id=5"); |
295 | - PerconaTest::wait_for_table($dbh, "osc.__new_t"); |
296 | - $dbh->do('use osc'); |
297 | - $dbh->do("DROP TABLE IF EXISTS osc.__new_t"); |
298 | + PerconaTest::wait_for_table($master_dbh, "osc.t", "id=5"); |
299 | + PerconaTest::wait_for_table($master_dbh, "osc.__new_t"); |
300 | + $master_dbh->do('use osc'); |
301 | + $master_dbh->do("DROP TABLE IF EXISTS osc.__new_t"); |
302 | |
303 | - $org_rows = $dbh->selectall_arrayref('select * from osc.t order by id'); |
304 | + $org_rows = $master_dbh->selectall_arrayref('select * from osc.t order by id'); |
305 | |
306 | output( |
307 | sub { $exit = pt_online_schema_change::main(@args, |
308 | 'D=osc,t=t', qw(--alter ENGINE=InnoDB)) }, |
309 | ); |
310 | |
311 | - $new_rows = $dbh->selectall_arrayref('select * from osc.t order by id'); |
312 | + $new_rows = $master_dbh->selectall_arrayref('select * from osc.t order by id'); |
313 | |
314 | is_deeply( |
315 | $new_rows, |
316 | @@ -280,5 +320,5 @@ |
317 | # ############################################################################# |
318 | # Done. |
319 | # ############################################################################# |
320 | -$sb->wipe_clean($dbh); |
321 | +$sb->wipe_clean($master_dbh); |
322 | exit; |
323 | |
324 | === modified file 't/pt-online-schema-change/option_sanity.t' |
325 | --- t/pt-online-schema-change/option_sanity.t 2011-07-12 22:56:55 +0000 |
326 | +++ t/pt-online-schema-change/option_sanity.t 2012-03-02 20:33:18 +0000 |
327 | @@ -9,7 +9,7 @@ |
328 | use strict; |
329 | use warnings FATAL => 'all'; |
330 | use English qw(-no_match_vars); |
331 | -use Test::More tests => 4; |
332 | +use Test::More tests => 5; |
333 | |
334 | use PerconaTest; |
335 | |
336 | @@ -44,6 +44,13 @@ |
337 | "Either DSN D part or --database required" |
338 | ); |
339 | |
340 | +$output = `$cmd --help`; |
341 | +like( |
342 | + $output, |
343 | + qr/--execute\s+FALSE/, |
344 | + "--execute FALSE by default" |
345 | +); |
346 | + |
347 | # ############################################################################# |
348 | # Done. |
349 | # ############################################################################# |