From 01daf0030e945a825729bbea065c08c8493383db Mon Sep 17 00:00:00 2001 From: Carlos Salguero Date: Wed, 22 Jun 2016 13:17:57 -0300 Subject: [PATCH 1/2] bug-1593265 Fixed pt-archiver deletes wrong rows In the case we are trying to migrate a table with no PK nor an unique key, pt-archiver was failing to select the correct rows for deletion. This fix implemented here is to add ALL columns in the WHERE clause of the DELETE command. This way, we are deleting only the exact same row we just migrated but using columns instead of an index. --- bin/pt-archiver | 2 +- bin/pt-online-schema-change | 2 +- bin/pt-table-checksum | 2 +- bin/pt-table-sync | 2 +- lib/TableNibbler.pm | 2 +- t/pt-archiver/issue_1166.t | 2 +- t/pt-archiver/issue_1593265.t | 64 +++++++++++++++++++++++++ t/pt-archiver/samples/issue_1593265.sql | 13 +++++ 8 files changed, 83 insertions(+), 6 deletions(-) create mode 100644 t/pt-archiver/issue_1593265.t create mode 100644 t/pt-archiver/samples/issue_1593265.sql diff --git a/bin/pt-archiver b/bin/pt-archiver index a061f0fa..13e9b415 100755 --- a/bin/pt-archiver +++ b/bin/pt-archiver @@ -3201,7 +3201,7 @@ sub generate_del_stmt { my $index = $tp->find_best_index($tbl, $args{index}); die "Cannot find an ascendable index in table" unless $index; - if ( $index ) { + if ( $index && $tbl->{keys}->{$index}->{is_unique}) { @del_cols = @{$tbl->{keys}->{$index}->{cols}}; } else { diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change index 7d96752b..20ad24e3 100755 --- a/bin/pt-online-schema-change +++ b/bin/pt-online-schema-change @@ -3052,7 +3052,7 @@ sub generate_del_stmt { my $index = $tp->find_best_index($tbl, $args{index}); die "Cannot find an ascendable index in table" unless $index; - if ( $index ) { + if ( $index && $tbl->{keys}->{$index}->{is_unique}) { @del_cols = @{$tbl->{keys}->{$index}->{cols}}; } else { diff --git a/bin/pt-table-checksum b/bin/pt-table-checksum index 6be21987..cb5efd80 100755 --- a/bin/pt-table-checksum +++ b/bin/pt-table-checksum @@ -4901,7 +4901,7 @@ sub generate_del_stmt { my $index = $tp->find_best_index($tbl, $args{index}); die "Cannot find an ascendable index in table" unless $index; - if ( $index ) { + if ( $index && $tbl->{keys}->{$index}->{is_unique}) { @del_cols = @{$tbl->{keys}->{$index}->{cols}}; } else { diff --git a/bin/pt-table-sync b/bin/pt-table-sync index 0db00dbd..d4819928 100755 --- a/bin/pt-table-sync +++ b/bin/pt-table-sync @@ -6480,7 +6480,7 @@ sub generate_del_stmt { my $index = $tp->find_best_index($tbl, $args{index}); die "Cannot find an ascendable index in table" unless $index; - if ( $index ) { + if ( $index && $tbl->{keys}->{$index}->{is_unique}) { @del_cols = @{$tbl->{keys}->{$index}->{cols}}; } else { diff --git a/lib/TableNibbler.pm b/lib/TableNibbler.pm index 0eed5177..945da85f 100644 --- a/lib/TableNibbler.pm +++ b/lib/TableNibbler.pm @@ -247,7 +247,7 @@ sub generate_del_stmt { die "Cannot find an ascendable index in table" unless $index; # These are the columns needed for the DELETE statement's WHERE clause. - if ( $index ) { + if ( $index && $tbl->{keys}->{$index}->{is_unique}) { @del_cols = @{$tbl->{keys}->{$index}->{cols}}; } else { diff --git a/t/pt-archiver/issue_1166.t b/t/pt-archiver/issue_1166.t index 749c90b6..fb511d3b 100644 --- a/t/pt-archiver/issue_1166.t +++ b/t/pt-archiver/issue_1166.t @@ -56,7 +56,7 @@ $output = output( ); like( $output, - qr/DELETE FROM `test`\.`issue_1166` WHERE \(`id` = \?\) LIMIT 1$/m, + qr/LIMIT 1$/m, "LIMIT 1 with non-unique index (issue 1166)" ); diff --git a/t/pt-archiver/issue_1593265.t b/t/pt-archiver/issue_1593265.t new file mode 100644 index 00000000..2ba5a340 --- /dev/null +++ b/t/pt-archiver/issue_1593265.t @@ -0,0 +1,64 @@ +#!/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 PerconaTest; +use Sandbox; +require "$trunk/bin/pt-archiver"; + +my $dp = new DSNParser(opts=>$dsn_opts); +my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp); +my $dbh = $sb->get_dbh_for('master'); + +if ( !$dbh ) { + plan skip_all => 'Cannot connect to sandbox master'; +} +else { + plan tests => 3; +} + +my $output; + +# ############################################################################# +# Issue 1152: mk-archiver columns option resulting in null archived table data +# ############################################################################# +$sb->load_file('master', 't/pt-archiver/samples/issue_1593265.sql'); + +$dbh->do('set names "utf8"'); + +$output = output( + sub { pt_archiver::main( + '--source', 'h=127.1,P=12345,D=test,t=t1,u=msandbox,p=msandbox', + '--dest', 't=t2', '--where', 'b in (1,2,3)') + }, +); + +my $untouched_rows = $dbh->selectall_arrayref('SELECT a, b FROM test.t1'); +is_deeply( + $untouched_rows, + [ ['10', '5'], ['10', '4'] ], + "Rows were left on the original table" +); + +my $new_rows = $dbh->selectall_arrayref('SELECT a, b FROM test.t2'); +is_deeply( + $new_rows, + [ ['10', '3'], ['10', '2'], ['10', '1'] ], + "Rows were archived into the new table" +); + +# ############################################################################# +# Done. +# ############################################################################# +$sb->wipe_clean($dbh); +ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox"); +exit; diff --git a/t/pt-archiver/samples/issue_1593265.sql b/t/pt-archiver/samples/issue_1593265.sql new file mode 100644 index 00000000..ac329ead --- /dev/null +++ b/t/pt-archiver/samples/issue_1593265.sql @@ -0,0 +1,13 @@ +CREATE DATABASE IF NOT EXISTS test; +USE test; +DROP TABLE IF EXISTS t1; +DROP TABLE IF EXISTS t2; + +create table t1 (a int, b int, key (a),key(b)); +create table t2 like t1; + +insert into t1 (a,b) values (10,5); +insert into t1 (a,b) values (10,4); +insert into t1 (a,b) values (10,3); +insert into t1 (a,b) values (10,2); +insert into t1 (a,b) values (10,1); From b018d09e4ea62118c9fc065df9d17b8131b5d24a Mon Sep 17 00:00:00 2001 From: Carlos Salguero Date: Wed, 22 Jun 2016 16:00:32 -0300 Subject: [PATCH 2/2] bug-1593265 Updated tests for TableNibbler.t --- t/lib/TableNibbler.t | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/t/lib/TableNibbler.t b/t/lib/TableNibbler.t index 51fe17cf..4df9a4a5 100644 --- a/t/lib/TableNibbler.t +++ b/t/lib/TableNibbler.t @@ -134,11 +134,13 @@ is_deeply( cols => [qw(film_id)], ), { - cols => [qw(film_id title)], + cols => [qw(film_id title description release_year language_id original_language_id rental_duration rental_rate length replacement_cost rating special_features last_update)], index => 'idx_title', - where => '(`title` = ?)', - slice => [1], - scols => [qw(title)], + where => '(`film_id` = ? AND `title` = ? AND ((? IS NULL AND `description` IS NULL) OR (`description` = ?)) AND ((? IS NULL AND `release_year` IS NULL) OR (`release_year` = ?)) AND `language_id` = ? AND ((? IS NULL AND `original_language_id` IS NULL) OR (`original_language_id` = ?)) AND `rental_duration` = ? AND `rental_rate` = ? AND ((? IS NULL AND `length` IS NULL) OR (`length` = ?)) AND `replacement_cost` = ? AND ((? IS NULL AND `rating` IS NULL) OR (`rating` = ?)) AND ((? IS NULL AND `special_features` IS NULL) OR (`special_features` = ?)) AND `last_update` = ?)', + slice => [ 0, 1, 2, 2, 3, 3, 4, 5, 5, 6, 7, 8, 8, 9, 10, 10, 11, 11, 12 ], + scols => [qw( film_id title description description release_year release_year language_id original_language_id original_language_id + rental_duration rental_rate length length replacement_cost rating rating special_features special_features last_update)], + }, 'del stmt on sakila.film with different index and extra column', );