From dc77289d603d1f006da02ade923e38803fd71b61 Mon Sep 17 00:00:00 2001 From: Perry Harrington Date: Tue, 28 Jan 2025 10:47:47 -0800 Subject: [PATCH 1/2] PT-2389 Fix unconditional ALTER on new table with resume When running with --resume option the ALTER runs on the new table even if the new table exists and has already been altered. This causes a deterministic failure every time the --resume option is used. This fix tests if --resume is given and does not run ALTER on the new table. If pt-osc did not successfully alter the new table during the previous invocation, this could cause the ALTER to not be applied. The right fix would be to compare the DDL of the _new table to the proposed DDL generated by pt-osc and only run ALTER if they do not match. --- bin/pt-online-schema-change | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index 9a960de16..6be7599df 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -9912,7 +9912,7 @@ sub main { ); } - if ( my $alter = $o->get('alter') ) { + if ( (my $alter = $o->get('alter')) && !$o->get('resume') ) { print "Altering new table...\n"; my $sql = "ALTER TABLE $new_tbl->{name} $alter"; print $sql, "\n" if $o->get('print'); From 888af5f5effeb6d37355bea1244bfef9617f0d93 Mon Sep 17 00:00:00 2001 From: Perry Harrington Date: Tue, 28 Jan 2025 11:51:03 -0800 Subject: [PATCH 2/2] Updated test for PT-1717 to perform meaningful alter so fix for PT-2389 would be exercised --- t/pt-online-schema-change/pt-1717-resume.t | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/t/pt-online-schema-change/pt-1717-resume.t b/t/pt-online-schema-change/pt-1717-resume.t index b44138f12..8930c6df5 100644 --- a/t/pt-online-schema-change/pt-1717-resume.t +++ b/t/pt-online-schema-change/pt-1717-resume.t @@ -137,7 +137,7 @@ set_delay(); # We need to sleep, otherwise pt-osc can finish before replica is delayed sleep($max_lag); -my $args = "$source_dsn,D=test,t=pt1717 --execute --chunk-size ${chunk_size} --max-lag $max_lag --alter 'engine=INNODB' --pid $tmp_file_name --progress time,5 --no-drop-new-table --no-drop-triggers --history"; +my $args = "$source_dsn,D=test,t=pt1717 --execute --chunk-size ${chunk_size} --max-lag $max_lag --alter 'ADD COLUMN foo varchar(32)' --pid $tmp_file_name --progress time,5 --no-drop-new-table --no-drop-triggers --history"; $output = run_broken_job($args); @@ -165,7 +165,7 @@ my @args = (qw(--execute --chunk-size=10 --history)); ($output, $exit) = full_output( sub { pt_online_schema_change::main(@args, "$source_dsn,D=test,t=pt1717", - '--alter', 'engine=INNODB', '--execute', "--resume=${job_id}", + '--alter', 'ADD COLUMN foo varchar(32)', '--execute', "--resume=${job_id}", '--chunk-index=f2' ) } ); @@ -186,7 +186,7 @@ like( sub { pt_online_schema_change::main(@args, "$source_dsn,D=test,t=pt1717", '--max-lag', $max_lag, '--resume', $job_id, - '--alter', 'engine=INNODB', + '--alter', 'ADD COLUMN foo varchar(32)', '--plugin', "$plugin/pt-1717.pm", ), }, @@ -209,7 +209,7 @@ ok( ) or diag("New table checksum: '${new_table_checksum}', original content checksum: '${old_table_checksum}'"); # Tests for chunk-index and chunk-index-columns options -$args = "$source_dsn,D=test,t=pt1717 --alter engine=innodb --execute --history --chunk-size=10 --no-drop-new-table --no-drop-triggers --reverse-triggers --chunk-index=f2"; +$args = "$source_dsn,D=test,t=pt1717 --alter 'ADD COLUMN foo varchar(32)' --execute --history --chunk-size=10 --no-drop-new-table --no-drop-triggers --reverse-triggers --chunk-index=f2"; set_delay(); $output = run_broken_job($args); @@ -220,7 +220,7 @@ $job_id = $1; ($output, $exit) = full_output( sub { pt_online_schema_change::main(@args, "$source_dsn,D=test,t=pt1717", - '--alter', 'engine=innodb', '--execute', "--resume=${job_id}", + '--alter', 'ADD COLUMN foo varchar(32)', '--execute', "--resume=${job_id}", ) } ); @@ -238,7 +238,7 @@ like( ($output, $exit) = full_output( sub { pt_online_schema_change::main(@args, "$source_dsn,D=test,t=pt1717", - '--alter', 'engine=innodb', '--execute', "--resume=${job_id}", + '--alter', 'ADD COLUMN foo varchar(32)', '--execute', "--resume=${job_id}", '--chunk-index=f1' ) } ); @@ -257,7 +257,7 @@ like( ($output, $exit) = full_output( sub { pt_online_schema_change::main(@args, "$source_dsn,D=test,t=pt1717", - '--alter', 'engine=innodb', '--execute', "--resume=${job_id}", + '--alter', 'ADD COLUMN foo varchar(32)', '--execute', "--resume=${job_id}", '--chunk-index=f2', '--chunk-index-columns=1' ) } ); @@ -300,7 +300,7 @@ is( ($output, $exit) = full_output( sub { pt_online_schema_change::main(@args, "$source_dsn,D=test,t=pt1717", - '--alter', 'engine=innodb', '--execute', "--resume=${job_id}", + '--alter', 'ADD COLUMN foo varchar(32)', '--execute', "--resume=${job_id}", '--chunk-size=4', '--chunk-index=f2' ) } @@ -348,7 +348,7 @@ ok( ($output, $exit) = full_output( sub { pt_online_schema_change::main(@args, "$source_dsn,D=test,t=pt1717", - '--alter', 'engine=innodb', '--execute', "--resume=${job_id}", + '--alter', 'ADD COLUMN foo varchar(32)', '--execute', "--resume=${job_id}", '--chunk-size=4', '--chunk-index=f2' ) } @@ -372,7 +372,7 @@ $output =~ /New table `test`.`([_]+pt1717_new)` not found, restart operation fro ($output, $exit) = full_output( sub { pt_online_schema_change::main(@args, "$source_dsn,D=test,t=pt1717", - '--alter', 'engine=innodb', '--execute', "--resume=${job_id}", + '--alter', 'ADD COLUMN foo varchar(32)', '--execute', "--resume=${job_id}", '--chunk-size=4', '--chunk-index=f2' ) }