Merge lp:~percona-toolkit-dev/percona-toolkit/chunk-index-columns-opt into lp:percona-toolkit/2.1

[Daniel Nichter]

[Daniel Nichter (daniel-nichter)]

review: Approve

[Baron Schwartz (baron-xaprb)]

review: Approve

=== modified file 'bin/pt-online-schema-change'
--- bin/pt-online-schema-change	2012-06-10 17:25:44 +0000
+++ bin/pt-online-schema-change	2012-06-11 12:18:22 +0000
@@ -1893,6 +1893,7 @@
       @asc_cols = $asc_cols[0];
    }
    elsif ( my $n = $args{n_index_cols} ) {
+      $n = scalar @asc_cols if $n > @asc_cols;
       PTDEBUG && _d('Ascending only first', $n, 'columns');
       @asc_cols = @asc_cols[0..($n-1)];
    }
@@ -5209,14 +5210,8 @@
       }
    }
 
-   # Parse --chunk-index INDEX:N where N is the number of
-   # left-most columns of INDEX to use.
    # https://bugs.launchpad.net/percona-toolkit/+bug/1010232
-   my ($chunk_index, $n_chunk_index_cols)
-      = split(':', $o->get('chunk-index') || '');
-   if ( defined $chunk_index && $chunk_index eq '' ) {
-      $o->save_error('--chunk-index cannot be an empty string');
-   }
+   my $n_chunk_index_cols = $o->get('chunk-index-columns');
    if ( defined $n_chunk_index_cols
         && (!$n_chunk_index_cols
             || $n_chunk_index_cols =~ m/\D/
@@ -6108,8 +6103,8 @@
       Cxn                => $cxn,
       tbl                => $orig_tbl,
       chunk_size         => $orig_tbl->{chunk_size},
-      chunk_index        => $chunk_index,
-      n_chunk_index_cols => $n_chunk_index_cols,
+      chunk_index        => $o->get('chunk-index'),
+      n_chunk_index_cols => $o->get('chunk-index-columns'),
       dml                => $dml,
       select             => $select,
       callbacks          => $callbacks,
@@ -7453,16 +7448,18 @@
 a C<FORCE INDEX> clause.  Be careful when using this option; a poor choice of
 index could cause bad performance.
 
-This option supports a special syntax to select a prefix of the index instead of
-the entire index. The syntax is NAME:N, where NAME is the name of the index, and
-N is the number of columns you wish to use. This works only for compound
-indexes, and is useful in cases where a bug in the MySQL query optimizer
-(planner) causes it to scan a large range of rows instead of using the index to
-locate starting and ending points precisely. This problem sometimes occurs on
-indexes with many columns, such as 4 or more. If this happens, the tool might
-print a warning related to the L<"--[no]check-plan"> option. Instructing
-the tool to use only the first N columns from the index is a workaround for
-the bug in some cases.
+=item --chunk-index-columns
+
+type: int
+
+Use only this many left-most columns of a L<"--chunk-index">.  This works
+only for compound indexes, and is useful in cases where a bug in the MySQL
+query optimizer (planner) causes it to scan a large range of rows instead
+of using the index to locate starting and ending points precisely.  This
+problem sometimes occurs on indexes with many columns, such as 4 or more.
+If this happens, the tool might print a warning related to the
+L<"--[no]check-plan"> option.  Instructing the tool to use only the first
+N columns of the index is a workaround for the bug in some cases.
 
 =item --chunk-size
 

=== modified file 'bin/pt-table-checksum'
--- bin/pt-table-checksum	2012-06-10 17:25:44 +0000
+++ bin/pt-table-checksum	2012-06-11 12:18:22 +0000
@@ -2294,6 +2294,7 @@
       @asc_cols = $asc_cols[0];
    }
    elsif ( my $n = $args{n_index_cols} ) {
+      $n = scalar @asc_cols if $n > @asc_cols;
       PTDEBUG && _d('Ascending only first', $n, 'columns');
       @asc_cols = @asc_cols[0..($n-1)];
    }
@@ -6170,14 +6171,8 @@
       }
    }
 
-   # Parse --chunk-index INDEX:N where N is the number of
-   # left-most columns of INDEX to use.
    # https://bugs.launchpad.net/percona-toolkit/+bug/1010232
-   my ($chunk_index, $n_chunk_index_cols)
-      = split(':', $o->get('chunk-index') || '');
-   if ( defined $chunk_index && $chunk_index eq '' ) {
-      $o->save_error('--chunk-index cannot be an empty string');
-   }
+   my $n_chunk_index_cols = $o->get('chunk-index-columns');
    if ( defined $n_chunk_index_cols
         && (!$n_chunk_index_cols
             || $n_chunk_index_cols =~ m/\D/
@@ -7116,8 +7111,8 @@
                Cxn                => $master_cxn,
                tbl                => $tbl,
                chunk_size         => $tbl->{chunk_size},
-               chunk_index        => $chunk_index,
-               n_chunk_index_cols => $n_chunk_index_cols,
+               chunk_index        => $o->get('chunk-index'),
+               n_chunk_index_cols => $o->get('chunk-index-columns'),
                dml                => $checksum_dml,
                select             => $checksum_cols,
                past_dml           => $checksum_dml,
@@ -8302,16 +8297,18 @@
 This is probably best to use when you are checksumming only a single table, not
 an entire server.
 
-This option supports a special syntax to select a prefix of the index instead of
-the entire index. The syntax is NAME:N, where NAME is the name of the index, and
-N is the number of columns you wish to use. This works only for compound
-indexes, and is useful in cases where a bug in the MySQL query optimizer
-(planner) causes it to scan a large range of rows instead of using the index to
-locate starting and ending points precisely. This problem sometimes occurs on
-indexes with many columns, such as 4 or more. If this happens, the tool might
-print a warning related to the L<"--[no]check-plan"> option. Instructing
-the tool to use only the first N columns from the index is a workaround for
-the bug in some cases.
+=item --chunk-index-columns
+
+type: int
+
+Use only this many left-most columns of a L<"--chunk-index">.  This works
+only for compound indexes, and is useful in cases where a bug in the MySQL
+query optimizer (planner) causes it to scan a large range of rows instead
+of using the index to locate starting and ending points precisely.  This
+problem sometimes occurs on indexes with many columns, such as 4 or more.
+If this happens, the tool might print a warning related to the
+L<"--[no]check-plan"> option.  Instructing the tool to use only the first
+N columns of the index is a workaround for the bug in some cases.
 
 =item --chunk-size
 

=== modified file 'lib/TableNibbler.pm'
--- lib/TableNibbler.pm	2012-06-10 14:01:25 +0000
+++ lib/TableNibbler.pm	2012-06-11 12:18:22 +0000
@@ -82,6 +82,7 @@
       @asc_cols = $asc_cols[0];
    }
    elsif ( my $n = $args{n_index_cols} ) {
+      $n = scalar @asc_cols if $n > @asc_cols;
       PTDEBUG && _d('Ascending only first', $n, 'columns');
       @asc_cols = @asc_cols[0..($n-1)];
    }

=== modified file 't/lib/TableNibbler.t'
--- t/lib/TableNibbler.t	2012-06-10 14:01:25 +0000
+++ t/lib/TableNibbler.t	2012-06-11 12:18:22 +0000
@@ -9,7 +9,7 @@
 use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
-use Test::More tests => 25;
+use Test::More tests => 26;
 
 use TableParser;
 use TableNibbler;
@@ -327,6 +327,39 @@
 
 is_deeply(
    $n->generate_asc_stmt(
+      tbl_struct   => $t,
+      cols         => $t->{cols},
+      index        => 'rental_date',
+      n_index_cols => 5,
+   ),
+   {
+      cols  => [qw(rental_id rental_date inventory_id customer_id
+                  return_date staff_id last_update)],
+      index => 'rental_date',
+      where => '((`rental_date` > ?) OR (`rental_date` = ? AND `inventory_id` > ?)'
+         . ' OR (`rental_date` = ? AND `inventory_id` = ? AND `customer_id` >= ?))',
+      slice => [1, 1, 2, 1, 2, 3],
+      scols => [qw(rental_date rental_date inventory_id rental_date inventory_id customer_id)],
+      boundaries => {
+         '>=' => '((`rental_date` > ?) OR (`rental_date` = ? AND '
+            . '`inventory_id` > ?) OR (`rental_date` = ? AND `inventory_id` '
+            . '= ? AND `customer_id` >= ?))',
+         '>' => '((`rental_date` > ?) OR (`rental_date` = ? AND '
+            . '`inventory_id` > ?) OR (`rental_date` = ? AND `inventory_id` '
+            . '= ? AND `customer_id` > ?))',
+         '<=' => '((`rental_date` < ?) OR (`rental_date` = ? AND '
+            . '`inventory_id` < ?) OR (`rental_date` = ? AND `inventory_id` '
+            . '= ? AND `customer_id` <= ?))',
+         '<' => '((`rental_date` < ?) OR (`rental_date` = ? AND '
+            . '`inventory_id` < ?) OR (`rental_date` = ? AND `inventory_id` '
+            . '= ? AND `customer_id` < ?))',
+      },
+   },
+   "Don't crash if N > number of index columns"
+);
+
+is_deeply(
+   $n->generate_asc_stmt(
       tbl_struct => $t,
       cols   => $t->{cols},
       index  => 'rental_date',

=== modified file 't/pt-table-checksum/chunk_index.t'
--- t/pt-table-checksum/chunk_index.t	2012-06-10 17:25:44 +0000
+++ t/pt-table-checksum/chunk_index.t	2012-06-11 12:18:22 +0000
@@ -25,7 +25,7 @@
    plan skip_all => 'Cannot connect to sandbox master';
 }
 else {
-   plan tests => 14;
+   plan tests => 16;
 }
 
 # The sandbox servers run with lock_wait_timeout=3 and it's not dynamic
@@ -193,11 +193,29 @@
          pt_table_checksum::main(
             $master_dsn, '--max-load', '',
             qw(--lock-wait-timeout 3 --chunk-size 5000  -t sakila.rental),
-            qw(--chunk-index rental_date:2 --explain --explain));
+            qw(--chunk-index rental_date --chunk-index-columns 2),
+            qw(--explain --explain));
       },
       "t/pt-table-checksum/samples/n-chunk-index-cols.txt",
    ),
-   "--chunk-index index:n"
+   "--chunk-index-columns"
+);
+
+$output = output(         
+   sub {
+      $exit_status = pt_table_checksum::main(
+         $master_dsn, '--max-load', '',
+         qw(--lock-wait-timeout 3 --chunk-size 5000  -t sakila.rental),
+         qw(--chunk-index rental_date --chunk-index-columns 5),
+         qw(--explain --explain));
+   },
+   stderr => 1,
+);
+
+is(
+   $exit_status,
+   0,
+   "--chunk-index-columns > number of index columns"
 );
 
 # #############################################################################