From f6f7876e17c2146707af6d2a5962d24c2cd1edb8 Mon Sep 17 00:00:00 2001 From: amontecillo Date: Tue, 11 Oct 2016 17:42:32 -0700 Subject: [PATCH 1/4] pt-osc: Fails with duplicate key in table for self-referencing FK https://bugs.launchpad.net/percona-toolkit/+bug/1632522 --- .gitignore | 1 + bin/pt-online-schema-change | 29 ++++++++++++++++++++++------- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 4ca924ce..88635435 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,4 @@ release snapshot .DS_Store build +Makefile.old diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index 4ec5feec..bf786d68 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -10093,11 +10093,22 @@ sub create_new_table { $sql =~ s/\ACREATE TABLE .*?\($/CREATE TABLE $quoted (/m; - # If it has a leading underscore, we remove one, otherwise we add one + # When the new temp table is created, we need to avoid collisions on constraint names # This is in contrast to previous behavior were we added underscores # indefinitely, sometimes exceeding the allowed name limit # https://bugs.launchpad.net/percona-toolkit/+bug/1215587 - $sql =~ s/^ CONSTRAINT `(_?)/' CONSTRAINT `'.($1 eq '' ? '_' : '')/gme; + # So we do replacements when constraint names: + # Has 2 _, we remove them + # Has 1 _, we add one to make 2 + # Has no _, we add one to make 1 + # This gives on more salt where the FK names have been previously been altered + # https://bugs.launchpad.net/percona-toolkit/+bug/1632522 + my %search = ( + 'CONSTRAINT `__' => 'CONSTRAINT `', + 'CONSTRAINT `_' => 'CONSTRAINT `__', + 'CONSTRAINT `' => 'CONSTRAINT `_' + ); + $sql =~ s/((?^:CONSTRAINT `__|CONSTRAINT `_|CONSTRAINT `))/$search{$1}/gm; if ( $o->get('default-engine') ) { $sql =~ s/\s+ENGINE=\S+//; @@ -10470,13 +10481,17 @@ sub rebuild_constraints { # This is in contrast to previous behavior were we added underscores # indefinitely, sometimes exceeding the allowed name limit # https://bugs.launchpad.net/percona-toolkit/+bug/1215587 - my $new_fk; + # Add one more salt to renaming FK constraint names + # This will add 2 _ to a self referencing FK thus avoiding a duplicate key constraint + # https://bugs.launchpad.net/percona-toolkit/+bug/1632522 + my $new_fk; if ($fk =~ /^_/) { - ($new_fk = $fk) =~ s/^_//; - }else { - $new_fk = '_'.$fk; + ($new_fk = $fk) =~ s/^_/__/; + } elsif ($fk =~ /^__/) { + ($new_fk = $fk) =~ s/^__//; + } else { + $new_fk = '_'.$fk; } - PTDEBUG && _d("Old FK name: $fk New FK name: $new_fk"); From 048defdb50efadc7b428f079df4782cd46af52d0 Mon Sep 17 00:00:00 2001 From: amontecillo Date: Wed, 12 Oct 2016 21:07:16 -0700 Subject: [PATCH 2/4] Simplified the new fk algo and renamed a variable --- bin/pt-online-schema-change | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index bf786d68..2cce5778 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -10103,12 +10103,12 @@ sub create_new_table { # Has no _, we add one to make 1 # This gives on more salt where the FK names have been previously been altered # https://bugs.launchpad.net/percona-toolkit/+bug/1632522 - my %search = ( + my %search_dict = ( 'CONSTRAINT `__' => 'CONSTRAINT `', 'CONSTRAINT `_' => 'CONSTRAINT `__', 'CONSTRAINT `' => 'CONSTRAINT `_' ); - $sql =~ s/((?^:CONSTRAINT `__|CONSTRAINT `_|CONSTRAINT `))/$search{$1}/gm; + $sql =~ s/((?^:CONSTRAINT `__|CONSTRAINT `_|CONSTRAINT `))/$search_dict{$1}/gm; if ( $o->get('default-engine') ) { $sql =~ s/\s+ENGINE=\S+//; @@ -10485,9 +10485,7 @@ sub rebuild_constraints { # This will add 2 _ to a self referencing FK thus avoiding a duplicate key constraint # https://bugs.launchpad.net/percona-toolkit/+bug/1632522 my $new_fk; - if ($fk =~ /^_/) { - ($new_fk = $fk) =~ s/^_/__/; - } elsif ($fk =~ /^__/) { + if ($fk =~ /^__/) { ($new_fk = $fk) =~ s/^__//; } else { $new_fk = '_'.$fk; From 5ea2f6b27fa2e8e2f6bd6f7bfcf318fde99710e2 Mon Sep 17 00:00:00 2001 From: amontecillo Date: Fri, 14 Oct 2016 17:12:07 -0700 Subject: [PATCH 3/4] Added tests for bug 1632522 Adjusted renamt_fk_constraints.t for new renaming logic --- .../rename_fk_constraints.t | 36 ++++-- .../rename_self_ref_fk_constraints.t | 113 ++++++++++++++++++ .../samples/bug-1632522.sql | 25 ++++ 3 files changed, 166 insertions(+), 8 deletions(-) create mode 100644 t/pt-online-schema-change/rename_self_ref_fk_constraints.t create mode 100644 t/pt-online-schema-change/samples/bug-1632522.sql diff --git a/t/pt-online-schema-change/rename_fk_constraints.t b/t/pt-online-schema-change/rename_fk_constraints.t index 1c8b13f5..f064ccfe 100644 --- a/t/pt-online-schema-change/rename_fk_constraints.t +++ b/t/pt-online-schema-change/rename_fk_constraints.t @@ -42,7 +42,7 @@ my $sample = "t/pt-online-schema-change/samples/"; $sb->load_file('master', "$sample/bug-1215587.sql"); # run once: we expect constraint names to be prefixed with one underscore -# if they havre't one, and to remove one if they already do. +# if they havre't one, and to remove 2 if they have 2 ($output, $exit_status) = full_output( sub { pt_online_schema_change::main(@args, "$master_dsn,D=bug1215587,t=Table1", @@ -50,25 +50,46 @@ $sb->load_file('master', "$sample/bug-1215587.sql"); qw(--execute)) }, ); - my $constraints = $master_dbh->selectall_arrayref("SELECT TABLE_NAME, CONSTRAINT_NAME FROM information_schema.KEY_COLUMN_USAGE WHERE table_schema='bug1215587' and (TABLE_NAME='Table1' OR TABLE_NAME='Table2') and CONSTRAINT_NAME LIKE '%fkey%' ORDER BY TABLE_NAME, CONSTRAINT_NAME"); is_deeply( $constraints, [ - ['Table1', 'fkey1a'], ['Table1', '_fkey1b'], - ['Table2', 'fkey2b'], + ['Table1', '__fkey1a'], ['Table2', '_fkey2a'], + ['Table2', '__fkey2b'], ], "First run adds or removes underscore from constraint names, accordingly" ); -# run second time -# we expect constraints to be the same as we started (toggled back) +# run second time: we expect constraint names to be prefixed with one underscore +# if they havre't one, and to remove 2 if they have 2 +($output, $exit_status) = full_output( + sub { pt_online_schema_change::main(@args, + "$master_dsn,D=bug1215587,t=Table1", + "--alter", "ENGINE=InnoDB", + qw(--execute)) }, +); + +$constraints = $master_dbh->selectall_arrayref("SELECT TABLE_NAME, CONSTRAINT_NAME FROM information_schema.KEY_COLUMN_USAGE WHERE table_schema='bug1215587' and (TABLE_NAME='Table1' OR TABLE_NAME='Table2') and CONSTRAINT_NAME LIKE '%fkey%' ORDER BY TABLE_NAME, CONSTRAINT_NAME"); + + +is_deeply( + $constraints, + [ + ['Table1', 'fkey1a'], + ['Table1', '__fkey1b'], + ['Table2', 'fkey2b'], + ['Table2', '__fkey2a'], + ], + "Second run adds or removes underscore from constraint names, accordingly" +); + +# run third time: we expect constraints to be the same as we started (toggled back) ($output, $exit_status) = full_output( sub { pt_online_schema_change::main(@args, "$master_dsn,D=bug1215587,t=Table1", @@ -87,10 +108,9 @@ is_deeply( ['Table2', 'fkey2a'], ['Table2', '_fkey2b'], ], - "Second run toggles constraint names back to how they were" + "Third run toggles constraint names back to how they were" ); - # ############################################################################# # Done. # ############################################################################# diff --git a/t/pt-online-schema-change/rename_self_ref_fk_constraints.t b/t/pt-online-schema-change/rename_self_ref_fk_constraints.t new file mode 100644 index 00000000..4b56d000 --- /dev/null +++ b/t/pt-online-schema-change/rename_self_ref_fk_constraints.t @@ -0,0 +1,113 @@ +#!/usr/bin/env perl + +BEGIN { + die "The PERCONA_TOOLKIT_BRANCH environment variable is not set.\n" + unless $ENV{PERCONA_TOOLKIT_BRANCH} && -d $ENV{PERCONA_TOOLKIT_BRANCH}; + unshift @INC, "$ENV{PERCONA_TOOLKIT_BRANCH}/lib"; +}; + +use strict; +use warnings FATAL => 'all'; +use English qw(-no_match_vars); +use Test::More; + +use Data::Dumper; +use PerconaTest; +use Sandbox; + +require "$trunk/bin/pt-online-schema-change"; + +my $dp = new DSNParser(opts=>$dsn_opts); +my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp); +my $master_dbh = $sb->get_dbh_for('master'); + +if ( !$master_dbh ) { + plan skip_all => 'Cannot connect to sandbox master'; +} + +# The sandbox servers run with lock_wait_timeout=3 and it's not dynamic +# so we need to specify --set-vars innodb_lock_wait_timeout-3 else the +# tool will die. +my $master_dsn = 'h=127.1,P=12345,u=msandbox,p=msandbox'; +my @args = (qw(--set-vars innodb_lock_wait_timeout=3 --alter-foreign-keys-method rebuild_constraints)); +my $output; +my $exit_status; +my $sample = "t/pt-online-schema-change/samples/"; + +# ############################################################################ +# https://bugs.launchpad.net/percona-toolkit/+bug/1632522 +# pt-online-schema-change fails with duplicate key in table for self-referencing FK +# ############################################################################ + +$sb->load_file('master', "$sample/bug-1632522.sql"); + +# run once: we expect the constraint name to be appended with one underscore +# but the self-referencing constraint will have 2 underscore +($output, $exit_status) = full_output( + sub { pt_online_schema_change::main(@args, + "$master_dsn,D=bug1632522,t=test_table", + "--alter", "ENGINE=InnoDB", + qw(--execute)) }, +); + +my $constraints = $master_dbh->selectall_arrayref("SELECT TABLE_NAME, CONSTRAINT_NAME FROM information_schema.KEY_COLUMN_USAGE WHERE table_schema='bug1632522' and (TABLE_NAME='test_table' OR TABLE_NAME='person') and CONSTRAINT_NAME LIKE '%fk_%' ORDER BY TABLE_NAME, CONSTRAINT_NAME"); + +is_deeply( + $constraints, + [ + ['person', '_fk_testId'], + ['test_table', '_fk_person'], + ['test_table', '__fk_refId'], + ], + "First run adds or removes underscore from constraint names, accordingly" +); + +# run second time: we expect constraint names to be prefixed with one underscore +# if they havre't one, and to remove 2 if they have 2 +($output, $exit_status) = full_output( + sub { pt_online_schema_change::main(@args, + "$master_dsn,D=bug1632522,t=test_table", + "--alter", "ENGINE=InnoDB", + qw(--execute)) }, +); + +$constraints = $master_dbh->selectall_arrayref("SELECT TABLE_NAME, CONSTRAINT_NAME FROM information_schema.KEY_COLUMN_USAGE WHERE table_schema='bug1632522' and (TABLE_NAME='test_table' OR TABLE_NAME='person') and CONSTRAINT_NAME LIKE '%fk_%' ORDER BY TABLE_NAME, CONSTRAINT_NAME"); + + +is_deeply( + $constraints, + [ + ['person', '__fk_testId'], + ['test_table', '_fk_refId'], + ['test_table', '__fk_person'], + ], + "Second run self-referencing will be one due to rebuild_constraints" +); + +# run third time: we expect constraints to be the same as we started (toggled back) +($output, $exit_status) = full_output( + sub { pt_online_schema_change::main(@args, + "$master_dsn,D=bug1632522,t=test_table", + "--alter", "ENGINE=InnoDB", + qw(--execute)) }, +); + +$constraints = $master_dbh->selectall_arrayref("SELECT TABLE_NAME, CONSTRAINT_NAME FROM information_schema.KEY_COLUMN_USAGE WHERE table_schema='bug1632522' and (TABLE_NAME='test_table' OR TABLE_NAME='person') and CONSTRAINT_NAME LIKE '%fk_%' ORDER BY TABLE_NAME, CONSTRAINT_NAME"); + + +is_deeply( + $constraints, + [ + ['person', 'fk_testId'], + ['test_table', 'fk_person'], + ['test_table', 'fk_refId'], + ], + "Third run toggles constraint names back to how they were" +); + +# ############################################################################# +# Done. +# ############################################################################# +$sb->wipe_clean($master_dbh); +ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox"); +done_testing; diff --git a/t/pt-online-schema-change/samples/bug-1632522.sql b/t/pt-online-schema-change/samples/bug-1632522.sql new file mode 100644 index 00000000..87ff3501 --- /dev/null +++ b/t/pt-online-schema-change/samples/bug-1632522.sql @@ -0,0 +1,25 @@ +-- Setup database and test tables with self referencing FK + +drop database if exists bug1632522; +create database bug1632522; +use bug1632522; + +CREATE TABLE `person` ( + `id` bigint(20) NOT NULL AUTO_INCREMENT, + `name` varchar(20) NOT NULL, + `testId` bigint(20) DEFAULT NULL, + PRIMARY KEY (`id`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +CREATE TABLE `test_table` ( + `id` bigint(20) NOT NULL AUTO_INCREMENT, + `refId` bigint(20) DEFAULT NULL, + `person` bigint(20) DEFAULT NULL, + PRIMARY KEY (`id`), + KEY `fk_person` (`person`), + KEY `fk_refId` (`refId`), + CONSTRAINT `fk_person` FOREIGN KEY (`person`) REFERENCES `person` (`id`), + CONSTRAINT `fk_refId` FOREIGN KEY (`refId`) REFERENCES `test_table` (`id`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +ALTER TABLE `person` ADD CONSTRAINT `fk_testId` FOREIGN KEY (`testId`) REFERENCES `test_table` (`id`); From 31fefa0605302ccbc57309fbf34eaacc34b0b18c Mon Sep 17 00:00:00 2001 From: Amiel Montecillo Date: Sun, 15 Jan 2017 21:06:16 -0800 Subject: [PATCH 4/4] Compatibility for older perl versions --- bin/pt-online-schema-change | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index 2cce5778..a542bd90 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -10108,7 +10108,8 @@ sub create_new_table { 'CONSTRAINT `_' => 'CONSTRAINT `__', 'CONSTRAINT `' => 'CONSTRAINT `_' ); - $sql =~ s/((?^:CONSTRAINT `__|CONSTRAINT `_|CONSTRAINT `))/$search_dict{$1}/gm; + my $constraint_pattern = qr((CONSTRAINT `__|CONSTRAINT `_|CONSTRAINT `)); + $sql =~ s/$constraint_pattern/$search_dict{$1}/gm; if ( $o->get('default-engine') ) { $sql =~ s/\s+ENGINE=\S+//;