Merge lp:~percona-toolkit-dev/percona-toolkit/pt-duplicate-key-checker-skips-report-when-verbose-on-1402730 into lp:~percona-toolkit-dev/percona-toolkit/release-2.2.14

[Frank Cizmich]

pt-duplicate-key-checker did not work when --verbose option was set
It simply seems an small error in the code made it skip the reporting functions when verbose was set.

Note: also tweaked test case to catch this.

Note2: also added warning about FULLTEXT indexes not being computed in total size, when they exist and are duplicated.

[David Bennett (dbpercona)]

Size of duplicate full text indexes is computed and reported as 0

Due to the variant nature of ft indexes, it would be best to avoid trying to compute the estimated size. I recommend changing the report line to:

  Size Duplicate Indexes ### (not including FULLTEXT indexes)

----

CREATE TABLE `ai5` (
  `id` int(11) NOT NULL,
  `c1` int(11) DEFAULT NULL,
  `c2` int(11) DEFAULT NULL,
  `c3` varchar(64) DEFAULT NULL,
  PRIMARY KEY (`id`),
  FULLTEXT KEY `c3` (`c3`),
  FULLTEXT KEY `c3_2` (`c3`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1

$ ./pt-duplicate-key-checker --verbose --tables=test.ai5
# ########################################################################
# test.ai5
# ########################################################################

# c3_2 (`c3`)
# PRIMARY (`id`)
# c3 (`c3`)

# c3_2 is a duplicate of c3
# Key definitions:
# FULLTEXT KEY `c3_2` (`c3`)
# FULLTEXT KEY `c3` (`c3`),
# Column types:
# `c3` varchar(64) default null
# To remove this duplicate index, execute:
ALTER TABLE `test`.`ai5` DROP INDEX `c3_2`;

# ########################################################################
# Summary of indexes
# ########################################################################

# Size Duplicate Indexes 0
# Total Duplicate Indexes 1
# Total Indexes 3

review: Needs Fixing

[David Bennett (dbpercona)]

Another edge case that fails, full column unique index duplicating partial unique column index, this should be reported.

---

cat <<EOF |
CREATE TABLE ai6 (
  id int(11) NOT NULL,
  c1 int(11) DEFAULT NULL,
  c2 int(11) DEFAULT NULL,
  c3 varchar(64) DEFAULT NULL,
  c4 varchar(64) DEFAULT NULL,
  PRIMARY KEY (id)
) ENGINE=InnoDB DEFAULT CHARSET=latin1;

-- OLD index entire column
CREATE UNIQUE INDEX c3_full ON ai6 (c3);

-- NEW enforce first 5 characters being unique
CREATE UNIQUE INDEX c3_partial ON ai6 (c3(5));

EOF
mysql -B test
bin/pt-duplicate-key-checker -d test -t ai6

review: Needs Fixing

[David Bennett (dbpercona)]

Previous comment (630899) appears to be an intermittent failure:

---

$ bin/pt-duplicate-key-checker -d test -t ai6
# ########################################################################
# test.ai6
# ########################################################################

# Uniqueness of c3_full ignored because c3_part is a duplicate constraint
# c3_full is a left-prefix of c3_part
# Key definitions:
# UNIQUE KEY `c3_full` (`c3`),
# UNIQUE KEY `c3_part` (`c3`(5))
# Column types:
# `c3` varchar(64) default null
# To remove this duplicate index, execute:
ALTER TABLE `test`.`ai6` DROP INDEX `c3_full`;

# ########################################################################
# Summary of indexes
# ########################################################################

# Size Duplicate Indexes 67
# Total Duplicate Indexes 1
# Total Indexes 3
$ bin/pt-duplicate-key-checker -d test -t ai6
# ########################################################################
# Summary of indexes
# ########################################################################

# Total Indexes 3
$ bin/pt-duplicate-key-checker -d test -t ai6
# ########################################################################
# Summary of indexes
# ########################################################################

# Total Indexes 3
$ bin/pt-duplicate-key-checker -d test -t ai6
# ########################################################################
# test.ai6
# ########################################################################

# Uniqueness of c3_full ignored because c3_part is a duplicate constraint
# c3_full is a left-prefix of c3_part
# Key definitions:
# UNIQUE KEY `c3_full` (`c3`),
# UNIQUE KEY `c3_part` (`c3`(5))
# Column types:
# `c3` varchar(64) default null
# To remove this duplicate index, execute:
ALTER TABLE `test`.`ai6` DROP INDEX `c3_full`;

# ########################################################################
# Summary of indexes
# ########################################################################

# Size Duplicate Indexes 67
# Total Duplicate Indexes 1
# Total Indexes 3

review: Needs Fixing

[Frank Cizmich (frank-cizmich)]

Reported intermitent issue as a separate bug.
https://bugs.launchpad.net/percona-toolkit/+bug/1436399

Let's narrow this fix to the original --verbose issue and that the tool issue a warning that FULLTEXT indexes are not computed in the reported total size of duplicates.

[lp:~percona-toolkit-dev/percona-toolkit/pt-duplicate-key-checker-skips-report-when-verbose-on-1402730]

615. By Frank Cizmich on 2015-03-25

Added warning about size of fulltext keys not being computed, if present. Sort key report by key name for ease of testing.

616. By Frank Cizmich on 2015-03-26

Added sort to some hash loops to make tests consistent. Modified test files accordingly

[Daniel Nichter (daniel-nichter)]

Did this fix become something else because I don't see where "verbose" is referenced in the diff below? I was expecting to see something like:

- if ( !$opts->{verbose) {
- report()
- }
+ report()

review: Needs Information

[Frank Cizmich (frank-cizmich)]

The "verbose" part was not visible in the context.

Old code:

            if ( $o->got('verbose') ) {
               print_all_keys($keys, $tbl, \%seen_tbl) if $keys;
               print_all_keys($fks, $tbl, \%seen_tbl) if $fks;
            }
            else {
               PTDEBUG && _d('Getting duplicate keys on',
                  $tbl->{db}, $tbl->{tbl});
                ....etc
New code:

            if ( $o->got('verbose') ) {
               print_all_keys($keys, $tbl, \%seen_tbl) if $keys;
               print_all_keys($fks, $tbl, \%seen_tbl) if $fks;
            }
            PTDEBUG && _d('Getting duplicate keys on',
               $tbl->{db}, $tbl->{tbl});
               ....etc

[Daniel Nichter (daniel-nichter)]

Ah, I see it now.

review: Approve

=== modified file 'bin/pt-duplicate-key-checker'
--- bin/pt-duplicate-key-checker	2015-01-23 10:19:56 +0000
+++ bin/pt-duplicate-key-checker	2015-03-26 18:43:21 +0000
@@ -2336,7 +2336,8 @@
    my @dupes;
 
    KEY:
-   foreach my $key ( values %keys ) {
+   # sort by key name for consistent testing
+   foreach my $key ( sort {$a->{name} cmp $b->{name}} values %keys ) {
       $key->{real_cols} = [ @{$key->{cols}} ];
 
       $key->{len_cols}  = length $key->{colnames};
@@ -2412,7 +2413,8 @@
 sub get_duplicate_fks {
    my ( $self, $fks, %args ) = @_;
    die "I need a fks argument" unless $fks;
-   my @fks = values %$fks;
+   # sort by name for consistent testing
+   my @fks = sort {$a->{name} cmp $b->{name}} values %$fks;
    my @dupes;
 
    foreach my $i ( 0..$#fks - 1 ) {
@@ -4881,7 +4883,7 @@
 sub main {
    local @ARGV = @_;  # set global ARGV for this package
 
-   my %summary = ( 'Total Indexes' => 0 );
+   my %summary = ( 'items' => {'Total Indexes' => 0} );
    my %seen_tbl;
 
    my $q  = new Quoter();
@@ -4972,51 +4974,49 @@
                print_all_keys($keys, $tbl, \%seen_tbl) if $keys;
                print_all_keys($fks,  $tbl, \%seen_tbl) if $fks;
             }
-            else {
-               PTDEBUG && _d('Getting duplicate keys on',
-                  $tbl->{db}, $tbl->{tbl});
-               if ( $keys ) {
-                  $dk->get_duplicate_keys(
-                     $keys,
-                     clustered_key => $clustered_key,
-                     tbl_info      => $tbl,
-                     callback      => \&print_duplicate_key,
-                     %tp_opts,
-                     # get_duplicate_keys() ignores these args but passes them
-                     # to the callback:
-                        dbh      => $dbh,
-                        is_fk    => 0,
-                        o        => $o,
-                        ks       => $ks,
-                        tp       => $tp,
-                        q        => $q,
-                        seen_tbl => \%seen_tbl,
-                        summary  => \%summary,
-                  );
-               }
-               if ( $fks ) {
-                  $dk->get_duplicate_fks(
-                     $fks,
-                     tbl_info => $tbl,
-                     callback => \&print_duplicate_key,
-                     %tp_opts,
-                     # get_duplicate_fks() ignores these args but passes them
-                     # to the callback:
-                        dbh   => $dbh,
-                        is_fk => 1,
-                        o     => $o,
-                        ks    => $ks,
-                        tp       => $tp,
-                        q        => $q,
-                        seen_tbl => \%seen_tbl,
-                        summary  => \%summary,
-                  );
-               }
+            PTDEBUG && _d('Getting duplicate keys on',
+               $tbl->{db}, $tbl->{tbl});
+            if ( $keys ) {
+               $dk->get_duplicate_keys(
+                  $keys,
+                  clustered_key => $clustered_key,
+                  tbl_info      => $tbl,
+                  callback      => \&print_duplicate_key,
+                  %tp_opts,
+                  # get_duplicate_keys() ignores these args but passes them
+                  # to the callback:
+                     dbh      => $dbh,
+                     is_fk    => 0,
+                     o        => $o,
+                     ks       => $ks,
+                     tp       => $tp,
+                     q        => $q,
+                     seen_tbl => \%seen_tbl,
+                     summary  => \%summary,
+               );
+            }
+            if ( $fks ) {
+               $dk->get_duplicate_fks(
+                  $fks,
+                  tbl_info => $tbl,
+                  callback => \&print_duplicate_key,
+                  %tp_opts,
+                  # get_duplicate_fks() ignores these args but passes them
+                  # to the callback:
+                     dbh   => $dbh,
+                     is_fk => 1,
+                     o     => $o,
+                     ks    => $ks,
+                     tp       => $tp,
+                     q        => $q,
+                     seen_tbl => \%seen_tbl,
+                     summary  => \%summary,
+               );
             }
 
             # Always count Total Keys so print_key_summary won't die
             # because %summary is empty.
-            $summary{'Total Indexes'} += (scalar keys %$keys)
+            $summary{items}->{'Total Indexes'} += (scalar keys %$keys)
                                        + (scalar keys %$fks)
          }
       };
@@ -5044,7 +5044,8 @@
       printf $hdr_fmt, "$db.$tbl";
       printf $hdr_fmt, ('#' x $hdr_width);
    }
-   foreach my $key ( values %$keys ) {
+   # Print keys sorted by name (easier to test)
+   foreach my $key (  sort {$a->{name} cmp $b->{name}} values %$keys ) {
       print "\n# $key->{name} ($key->{colnames})";
    }
    print "\n";
@@ -5069,6 +5070,10 @@
    my $summary  = $args{summary};
    my $struct   = $tp->parse($args{tbl_info}->{ddl});
 
+   if ($dupe->{ddl} =~ /FULLTEXT/) {
+      $summary->{has_fulltext_dupe}++;
+   }
+
    if ( !$seen_tbl->{"$db$tbl"}++ ) {
       printf $hdr_fmt, ('#' x $hdr_width);
       printf $hdr_fmt, "$db.$tbl";
@@ -5111,7 +5116,7 @@
    print "\n";
 
    if ( $o->get('summary') && $summary ) {
-      $summary->{'Total Duplicate Indexes'} += 1;
+      $summary->{'items'}->{'Total Duplicate Indexes'} += 1;
       my ($size, $chosen_key) = $ks->get_key_size(
          name        => $dupe->{key},
          cols        => $dupe->{cols},
@@ -5128,7 +5133,7 @@
          $size ||= 0;
 
          # Create Size Duplicate Keys summary even if there's no valid keys.
-         $summary->{'Size Duplicate Indexes'} += $size;
+         $summary->{'items'}->{'Size Duplicate Indexes'} += $size;
 
          if ( $size ) {
             if ( $chosen_key && $chosen_key ne $dupe->{key} ) {
@@ -5143,14 +5148,19 @@
 
 sub print_key_summary {
    my ( %summary ) = @_;
+   my $items = $summary{items};
    printf $hdr_fmt, ('#' x $hdr_width);
    printf $hdr_fmt, 'Summary of indexes';
    printf $hdr_fmt, ('#' x $hdr_width);
    print "\n";
-   my $max_item = max(map { length($_) } keys %summary);
-   my $line_fmt = "# %-${max_item}s  %-s\n";
-   foreach my $item ( sort keys %summary ) {
-      printf $line_fmt, $item, $summary{$item};
+   my $max_item = max(map { length($_) } keys %$items);
+   my $line_fmt = "# %-${max_item}s  %-s";
+   foreach my $item ( sort keys %$items ) {
+      printf $line_fmt, $item, $items->{$item};
+      if ( $item eq 'Size Duplicate Indexes' && $summary{has_fulltext_dupe} ) {
+         print ' (not including FULLTEXT indexes)';
+      }
+      print "\n";
    }
    return;
 }

=== modified file 't/pt-duplicate-key-checker/basics.t'
--- t/pt-duplicate-key-checker/basics.t	2013-12-12 03:39:09 +0000
+++ t/pt-duplicate-key-checker/basics.t	2015-03-26 18:43:21 +0000
@@ -142,12 +142,25 @@
 
 ok(
    no_diff(
-      sub { pt_duplicate_key_checker::main(@args, qw(-t test.domains)) },
-      "$sample/simple_dupe_bug_1217013.txt"),
+      sub { pt_duplicate_key_checker::main(@args, qw(-t test.domains -v)) },
+      "$sample/simple_dupe_bug_1217013.txt", keep_output=>1),
    'Exact unique dupes (bug 1217013)'
 ) or diag($test_diff);
 
 # #############################################################################
+# Same thing, but added --verbose option to test for:
+# https://bugs.launchpad.net/percona-toolkit/+bug/1402730
+# #############################################################################
+
+ok(
+   no_diff(
+      sub { pt_duplicate_key_checker::main(@args, qw(-t test.domains --verbose)) },
+      "$sample/simple_dupe_bug_1217013.txt", keep_output=>1),
+   q[--verbose option doesn't skip dupes reporting (bug 1402730)]
+) or diag($test_diff);
+
+
+# #############################################################################
 # Done.
 # #############################################################################
 $sb->wipe_clean($dbh);

=== modified file 't/pt-duplicate-key-checker/issue_331.t'
--- t/pt-duplicate-key-checker/issue_331.t	2012-06-03 19:14:30 +0000
+++ t/pt-duplicate-key-checker/issue_331.t	2015-03-26 18:43:21 +0000
@@ -41,10 +41,10 @@
 ok(
    no_diff(
       sub { pt_duplicate_key_checker::main(@args, qw(-d issue_331)) },
-      't/pt-duplicate-key-checker/samples/issue_331.txt',
+      't/pt-duplicate-key-checker/samples/issue_331.txt'
    ),
    'Issue 331 crash on fks'
-);
+) or diag($test_diff);
 
 # #############################################################################
 # Done.

=== modified file 't/pt-duplicate-key-checker/samples/issue_331.txt'
--- t/pt-duplicate-key-checker/samples/issue_331.txt	2011-06-24 22:02:05 +0000
+++ t/pt-duplicate-key-checker/samples/issue_331.txt	2015-03-26 18:43:21 +0000
@@ -2,14 +2,14 @@
 # issue_331.issue_331_t2                                                  
 # ########################################################################
 
-# FOREIGN KEY fk_1 (`id`) REFERENCES `issue_331`.`issue_331_t1` (`t1_id`) is a duplicate of FOREIGN KEY fk_2 (`id`) REFERENCES `issue_331`.`issue_331_t1` (`t1_id`)
+# FOREIGN KEY fk_2 (`id`) REFERENCES `issue_331`.`issue_331_t1` (`t1_id`) is a duplicate of FOREIGN KEY fk_1 (`id`) REFERENCES `issue_331`.`issue_331_t1` (`t1_id`)
 # Key definitions:
+#   CONSTRAINT `fk_2` FOREIGN KEY (`id`) REFERENCES `issue_331_t1` (`t1_id`)
 #   CONSTRAINT `fk_1` FOREIGN KEY (`id`) REFERENCES `issue_331_t1` (`t1_id`)
-#   CONSTRAINT `fk_2` FOREIGN KEY (`id`) REFERENCES `issue_331_t1` (`t1_id`)
 # Column types:
 #	  `id` bigint(20) not null default '0'
 # To remove this duplicate foreign key, execute:
-ALTER TABLE `issue_331`.`issue_331_t2` DROP FOREIGN KEY `fk_1`;
+ALTER TABLE `issue_331`.`issue_331_t2` DROP FOREIGN KEY `fk_2`;
 
 # MySQL uses the PRIMARY index for this foreign key constraint
 

=== modified file 't/pt-duplicate-key-checker/samples/simple_dupe_bug_1217013.txt'
--- t/pt-duplicate-key-checker/samples/simple_dupe_bug_1217013.txt	2013-12-12 03:39:09 +0000
+++ t/pt-duplicate-key-checker/samples/simple_dupe_bug_1217013.txt	2015-03-26 18:43:21 +0000
@@ -2,15 +2,19 @@
 # test.domains                                                            
 # ########################################################################
 
-# Uniqueness of domain ignored because unique_key_domain is a duplicate constraint
-# domain is a duplicate of unique_key_domain
+# PRIMARY (`id`)
+# domain (`domain`)
+# unique_key_domain (`domain`)
+
+# Uniqueness of unique_key_domain ignored because domain is a duplicate constraint
+# unique_key_domain is a duplicate of domain
 # Key definitions:
+#   UNIQUE KEY `unique_key_domain` (`domain`)
 #   UNIQUE KEY `domain` (`domain`),
-#   UNIQUE KEY `unique_key_domain` (`domain`)
 # Column types:
 #	  `domain` varchar(175) collate utf8_bin not null
 # To remove this duplicate index, execute:
-ALTER TABLE `test`.`domains` DROP INDEX `domain`;
+ALTER TABLE `test`.`domains` DROP INDEX `unique_key_domain`;
 
 # ########################################################################
 # Summary of indexes