Merge lp:~percona-toolkit-dev/percona-toolkit/ptc-errors-on-slave-with-different-time-zone-1388870 into lp:~percona-toolkit-dev/percona-toolkit/release-2.2.13

[Frank Cizmich]

Problem:
  When a slave is in a server with a different system timezone, pt-table-checksum detects differences on timestamp type columns.

Solution:
 Normalize the output of those columns using UNIX_TIMESTAMP

Notes:
 - No difference in performance was detected. Docs say it is an extremely fast function.
 - Tested manually. Creating a test case for this is near impossible since system tz is read-only on the database, and is lifted from the machine at startup.

[Daniel Nichter (daniel-nichter)]

I think this is ok. The +0 was just a clever way to coerce a timestamp to a big int. UNIX_TIMESTAMP() does the same but avoids the tz problem. So the only thing this MP lacks is a test case. This should be possible because you can SET GLOBAL time_zone='+7:00' on a slave and then reproduce the problem. We should not get in habit of fixes without solid test cases.

review: Needs Fixing

[Frank Cizmich (frank-cizmich)]

> I think this is ok. The +0 was just a clever way to coerce a timestamp to a
> big int. UNIX_TIMESTAMP() does the same but avoids the tz problem. So the only
> thing this MP lacks is a test case. This should be possible because you can
> SET GLOBAL time_zone='+7:00' on a slave and then reproduce the problem. We
> should not get in habit of fixes without solid test cases.

Believe me I tried! But this is some sort of subtle issue.
A change in global time_zone alone does not reproduce the problem.
It's only when you change the system time_zone that the issue arises, (and it is solved by using UNXIX_TIMESTAMP)
To change the system_time_zone , the only way is to shut down the slave, change the machine time_zone, and boot slave again.
That's the only way I could reproduce the bug.

I think it's something to do with the way INSERT...SELECT handles timezones. When reading data from slaves it prefers system_time_zone over (global) time_zone.

Again, timestamps are stored correctly, and UNIX_TIMESTAMP fixes the issue.

Just that an automated test case seems near impossible.

[Daniel Nichter (daniel-nichter)]

Can do TZ="UTC" <start slave> to set system_time_zone on startup, so this can be tested.

[Frank Cizmich (frank-cizmich)]

Just to add that, also according to my tests, indeed it's a replication thing.

When doing insert on master, slave uses system_tz to do convert to UTC locally when inserting into a timestamp column.
When doing insert..select on master, slave also uses its own system_tz to convert to UTC locally when reading.

[lp:~percona-toolkit-dev/percona-toolkit/ptc-errors-on-slave-with-different-time-zone-1388870]

613. By Frank Cizmich on 2015-01-19

pt-tcs added testcase for timezone bug

=== modified file 'bin/pt-table-checksum'
--- bin/pt-table-checksum	2014-11-11 13:28:27 +0000
+++ bin/pt-table-checksum	2015-01-19 22:19:29 +0000
@@ -5714,8 +5714,8 @@
       $query = join(', ',
                   map { 
                      my $col = $_;
-                     if ( $col =~ m/\+ 0/ ) {
-                        my ($real_col) = /^(\S+)/;
+                     if ( $col =~ m/UNIX_TIMESTAMP/ ) {
+                        my ($real_col) = /^UNIX_TIMESTAMP\((.+?)\)/;
                         $col .= " AS $real_col";
                      }
                      elsif ( $col =~ m/TRIM/ ) {
@@ -5815,7 +5815,7 @@
          my $type   = $tbl_struct->{type_for}->{$_};
          my $result = $q->quote($_);
          if ( $type eq 'timestamp' ) {
-            $result .= ' + 0';
+            $result = "UNIX_TIMESTAMP($result)";
          }
          elsif ( $float_precision && $type =~ m/float|double/ ) {
             $result = "ROUND($result, $float_precision)";
 
=== modified file 'lib/RowChecksum.pm'
--- lib/RowChecksum.pm	2014-08-05 14:21:32 +0000
+++ lib/RowChecksum.pm	2015-01-19 22:19:29 +0000
@@ -82,10 +82,10 @@
       $query = join(', ',
                   map { 
                      my $col = $_;
-                     if ( $col =~ m/\+ 0/ ) {
+                     if ( $col =~ m/UNIX_TIMESTAMP/ ) {
                         # Alias col name back to itself else its name becomes
                         # "col + 0" instead of just "col".
-                        my ($real_col) = /^(\S+)/;
+                        my ($real_col) = /^UNIX_TIMESTAMP\((.+?)\)/;
                         $col .= " AS $real_col";
                      }
                      elsif ( $col =~ m/TRIM/ ) {
@@ -216,7 +216,7 @@
          my $type   = $tbl_struct->{type_for}->{$_};
          my $result = $q->quote($_);
          if ( $type eq 'timestamp' ) {
-            $result .= ' + 0';
+            $result = "UNIX_TIMESTAMP($result)";
          }
          elsif ( $float_precision && $type =~ m/float|double/ ) {
             $result = "ROUND($result, $float_precision)";
 
=== modified file 't/lib/RowChecksum.t'
--- t/lib/RowChecksum.t	2012-10-30 15:38:31 +0000
+++ t/lib/RowChecksum.t	2015-01-19 22:19:29 +0000
@@ -126,11 +126,11 @@
       tbl  => $tbl,
       func => 'SHA1',
    ),
-     q{`film_id`, `title`, `description`, `release_year`, `language_id`, `original_language_id`, `rental_duration`, `rental_rate`, `length`, `replacement_cost`, `rating`, `special_features`, `last_update` + 0 AS `last_update`, }
+     q{`film_id`, `title`, `description`, `release_year`, `language_id`, `original_language_id`, `rental_duration`, `rental_rate`, `length`, `replacement_cost`, `rating`, `special_features`, UNIX_TIMESTAMP(`last_update`) AS `last_update`, }
    . q{SHA1(CONCAT_WS('#', }
    . q{`film_id`, `title`, `description`, `release_year`, `language_id`, }
    . q{`original_language_id`, `rental_duration`, `rental_rate`, `length`, }
-   . q{`replacement_cost`, `rating`, `special_features`, `last_update` + 0, }
+   . q{`replacement_cost`, `rating`, `special_features`, UNIX_TIMESTAMP(`last_update`), }
    . q{CONCAT(ISNULL(`description`), ISNULL(`release_year`), }
    . q{ISNULL(`original_language_id`), ISNULL(`length`), }
    . q{ISNULL(`rating`), ISNULL(`special_features`))))},
@@ -142,11 +142,11 @@
       tbl  => $tbl,
       func => 'FNV_64',
    ),
-     q{`film_id`, `title`, `description`, `release_year`, `language_id`, `original_language_id`, `rental_duration`, `rental_rate`, `length`, `replacement_cost`, `rating`, `special_features`, `last_update` + 0 AS `last_update`, }
+     q{`film_id`, `title`, `description`, `release_year`, `language_id`, `original_language_id`, `rental_duration`, `rental_rate`, `length`, `replacement_cost`, `rating`, `special_features`, UNIX_TIMESTAMP(`last_update`) AS `last_update`, }
    . q{FNV_64(}
    . q{`film_id`, `title`, `description`, `release_year`, `language_id`, }
    . q{`original_language_id`, `rental_duration`, `rental_rate`, `length`, }
-   . q{`replacement_cost`, `rating`, `special_features`, `last_update` + 0)},
+   . q{`replacement_cost`, `rating`, `special_features`, UNIX_TIMESTAMP(`last_update`))},
    'FNV_64 query for sakila.film',
 );
 
 
=== modified file 't/pt-table-checksum/bugs.t'
--- t/pt-table-checksum/bugs.t	2013-10-02 18:26:11 +0000
+++ t/pt-table-checksum/bugs.t	2015-01-19 22:19:29 +0000
@@ -295,6 +295,31 @@
 );
 
 # #############################################################################
+# pt-table-checksum has errors when slaves have different system_time_zone 
+# https://bugs.launchpad.net/percona-toolkit/+bug/1388870 
+# #############################################################################
+
+# make slave set diferent system_time_zone by changing env var TZ. 
+diag(`/tmp/12346/stop >/dev/null`); 
+diag(`export TZ='HST';/tmp/12346/start >/dev/null`); 
+
+$output = output(
+   sub { pt_table_checksum::main(@args, qw(-t sakila.payment)) },
+);
+
+
+is(
+   PerconaTest::count_checksum_results($output, 'diffs'),
+   0,
+   "Bug 1388870 - No false positive reported when system_tz differ on slave"
+);
+
+# restore slave to original system_tz 
+diag(`/tmp/12346/stop >/dev/null`); 
+diag(`/tmp/12346/start >/dev/null`); 
+
+
+# #############################################################################
 # Done.
 # #############################################################################
 $sb->wipe_clean($master_dbh);
 
=== modified file 't/pt-table-checksum/samples/chunkidx004.txt'
--- t/pt-table-checksum/samples/chunkidx004.txt	2011-12-27 18:12:40 +0000
+++ t/pt-table-checksum/samples/chunkidx004.txt	2015-01-19 22:19:29 +0000
@@ -2,7 +2,7 @@
 -- sakila.city
 --
 
-REPLACE INTO `percona`.`checksums` (db, tbl, chunk, chunk_index, lower_boundary, upper_boundary, this_cnt, this_crc) SELECT ?, ?, ?, ?, ?, ?, COUNT(*) AS cnt, COALESCE(LOWER(CONV(BIT_XOR(CAST(CRC32(CONCAT_WS('#', `city_id`, `city`, `country_id`, `last_update` + 0)) AS UNSIGNED)), 10, 16)), 0) AS crc FROM `sakila`.`city` FORCE INDEX(`idx_fk_country_id`) WHERE ((`country_id` >= ?)) AND ((`country_id` <= ?)) AND (country_id > 100) /*checksum chunk*/
+REPLACE INTO `percona`.`checksums` (db, tbl, chunk, chunk_index, lower_boundary, upper_boundary, this_cnt, this_crc) SELECT ?, ?, ?, ?, ?, ?, COUNT(*) AS cnt, COALESCE(LOWER(CONV(BIT_XOR(CAST(CRC32(CONCAT_WS('#', `city_id`, `city`, `country_id`, UNIX_TIMESTAMP(`last_update`))) AS UNSIGNED)), 10, 16)), 0) AS crc FROM `sakila`.`city` FORCE INDEX(`idx_fk_country_id`) WHERE ((`country_id` >= ?)) AND ((`country_id` <= ?)) AND (country_id > 100) /*checksum chunk*/
 
 REPLACE INTO `percona`.`checksums` (db, tbl, chunk, chunk_index, lower_boundary, upper_boundary, this_cnt, this_crc) SELECT ?, ?, ?, ?, ?, ?, COUNT(*), '0' FROM `sakila`.`city` FORCE INDEX(`idx_fk_country_id`) WHERE ((`country_id` < ?)) AND (country_id > 100) ORDER BY `country_id` /*past lower chunk*/
 
 
=== modified file 't/pt-table-checksum/samples/chunkidx005.txt'
--- t/pt-table-checksum/samples/chunkidx005.txt	2011-12-27 18:12:40 +0000
+++ t/pt-table-checksum/samples/chunkidx005.txt	2015-01-19 22:19:29 +0000
@@ -2,7 +2,7 @@
 -- sakila.city
 --
 
-REPLACE INTO `percona`.`checksums` (db, tbl, chunk, chunk_index, lower_boundary, upper_boundary, this_cnt, this_crc) SELECT ?, ?, ?, ?, ?, ?, COUNT(*) AS cnt, COALESCE(LOWER(CONV(BIT_XOR(CAST(CRC32(CONCAT_WS('#', `city_id`, `city`, `country_id`, `last_update` + 0)) AS UNSIGNED)), 10, 16)), 0) AS crc FROM `sakila`.`city` FORCE INDEX(`PRIMARY`) WHERE ((`city_id` >= ?)) AND ((`city_id` <= ?)) AND (country_id > 100) /*checksum chunk*/
+REPLACE INTO `percona`.`checksums` (db, tbl, chunk, chunk_index, lower_boundary, upper_boundary, this_cnt, this_crc) SELECT ?, ?, ?, ?, ?, ?, COUNT(*) AS cnt, COALESCE(LOWER(CONV(BIT_XOR(CAST(CRC32(CONCAT_WS('#', `city_id`, `city`, `country_id`, UNIX_TIMESTAMP(`last_update`))) AS UNSIGNED)), 10, 16)), 0) AS crc FROM `sakila`.`city` FORCE INDEX(`PRIMARY`) WHERE ((`city_id` >= ?)) AND ((`city_id` <= ?)) AND (country_id > 100) /*checksum chunk*/
 
 REPLACE INTO `percona`.`checksums` (db, tbl, chunk, chunk_index, lower_boundary, upper_boundary, this_cnt, this_crc) SELECT ?, ?, ?, ?, ?, ?, COUNT(*), '0' FROM `sakila`.`city` FORCE INDEX(`PRIMARY`) WHERE ((`city_id` < ?)) AND (country_id > 100) ORDER BY `city_id` /*past lower chunk*/
 
 
=== modified file 't/pt-table-checksum/samples/n-chunk-index-cols.txt'
--- t/pt-table-checksum/samples/n-chunk-index-cols.txt	2012-06-10 14:43:42 +0000
+++ t/pt-table-checksum/samples/n-chunk-index-cols.txt	2015-01-19 22:19:29 +0000
@@ -2,7 +2,7 @@
 -- sakila.rental
 --
 
-REPLACE INTO `percona`.`checksums` (db, tbl, chunk, chunk_index, lower_boundary, upper_boundary, this_cnt, this_crc) SELECT ?, ?, ?, ?, ?, ?, COUNT(*) AS cnt, COALESCE(LOWER(CONV(BIT_XOR(CAST(CRC32(CONCAT_WS('#', `rental_id`, `rental_date`, `inventory_id`, `customer_id`, `return_date`, `staff_id`, `last_update` + 0, CONCAT(ISNULL(`return_date`)))) AS UNSIGNED)), 10, 16)), 0) AS crc FROM `sakila`.`rental` FORCE INDEX(`rental_date`) WHERE ((`rental_date` > ?) OR (`rental_date` = ? AND `inventory_id` >= ?)) AND ((`rental_date` < ?) OR (`rental_date` = ? AND `inventory_id` <= ?)) /*checksum chunk*/
+REPLACE INTO `percona`.`checksums` (db, tbl, chunk, chunk_index, lower_boundary, upper_boundary, this_cnt, this_crc) SELECT ?, ?, ?, ?, ?, ?, COUNT(*) AS cnt, COALESCE(LOWER(CONV(BIT_XOR(CAST(CRC32(CONCAT_WS('#', `rental_id`, `rental_date`, `inventory_id`, `customer_id`, `return_date`, `staff_id`, UNIX_TIMESTAMP(`last_update`), CONCAT(ISNULL(`return_date`)))) AS UNSIGNED)), 10, 16)), 0) AS crc FROM `sakila`.`rental` FORCE INDEX(`rental_date`) WHERE ((`rental_date` > ?) OR (`rental_date` = ? AND `inventory_id` >= ?)) AND ((`rental_date` < ?) OR (`rental_date` = ? AND `inventory_id` <= ?)) /*checksum chunk*/
 
 REPLACE INTO `percona`.`checksums` (db, tbl, chunk, chunk_index, lower_boundary, upper_boundary, this_cnt, this_crc) SELECT ?, ?, ?, ?, ?, ?, COUNT(*), '0' FROM `sakila`.`rental` FORCE INDEX(`rental_date`) WHERE ((`rental_date` < ?) OR (`rental_date` = ? AND `inventory_id` < ?)) ORDER BY `rental_date`, `inventory_id`, `customer_id` /*past lower chunk*/