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

Proposed by Frank Cizmich
Status: Merged
Approved by: Frank Cizmich
Approved revision: 613
Merged at revision: 621
Proposed branch: lp:~percona-toolkit-dev/percona-toolkit/ptc-errors-on-slave-with-different-time-zone-1388870
Merge into: lp:~percona-toolkit-dev/percona-toolkit/release-2.2.13
Diff against target: 155 lines (+38/-13)
7 files modified
bin/pt-table-checksum (+3/-3)
lib/RowChecksum.pm (+3/-3)
t/lib/RowChecksum.t (+4/-4)
t/pt-table-checksum/bugs.t (+25/-0)
t/pt-table-checksum/samples/chunkidx004.txt (+1/-1)
t/pt-table-checksum/samples/chunkidx005.txt (+1/-1)
t/pt-table-checksum/samples/n-chunk-index-cols.txt (+1/-1)
To merge this branch: bzr merge lp:~percona-toolkit-dev/percona-toolkit/ptc-errors-on-slave-with-different-time-zone-1388870
Reviewer Review Type Date Requested Status
Daniel Nichter Needs Fixing
Review via email: mp+245683@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Daniel Nichter (daniel-nichter) wrote :

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
Revision history for this message
Frank Cizmich (frank-cizmich) wrote :

> 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.

Revision history for this message
Daniel Nichter (daniel-nichter) wrote :

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

Revision history for this message
Frank Cizmich (frank-cizmich) wrote :

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.

613. By Frank Cizmich

pt-tcs added testcase for timezone bug

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/pt-table-checksum'
2--- bin/pt-table-checksum 2014-11-11 13:28:27 +0000
3+++ bin/pt-table-checksum 2015-01-19 22:19:29 +0000
4@@ -5714,8 +5714,8 @@
5 $query = join(', ',
6 map {
7 my $col = $_;
8- if ( $col =~ m/\+ 0/ ) {
9- my ($real_col) = /^(\S+)/;
10+ if ( $col =~ m/UNIX_TIMESTAMP/ ) {
11+ my ($real_col) = /^UNIX_TIMESTAMP\((.+?)\)/;
12 $col .= " AS $real_col";
13 }
14 elsif ( $col =~ m/TRIM/ ) {
15@@ -5815,7 +5815,7 @@
16 my $type = $tbl_struct->{type_for}->{$_};
17 my $result = $q->quote($_);
18 if ( $type eq 'timestamp' ) {
19- $result .= ' + 0';
20+ $result = "UNIX_TIMESTAMP($result)";
21 }
22 elsif ( $float_precision && $type =~ m/float|double/ ) {
23 $result = "ROUND($result, $float_precision)";
24
25=== modified file 'lib/RowChecksum.pm'
26--- lib/RowChecksum.pm 2014-08-05 14:21:32 +0000
27+++ lib/RowChecksum.pm 2015-01-19 22:19:29 +0000
28@@ -82,10 +82,10 @@
29 $query = join(', ',
30 map {
31 my $col = $_;
32- if ( $col =~ m/\+ 0/ ) {
33+ if ( $col =~ m/UNIX_TIMESTAMP/ ) {
34 # Alias col name back to itself else its name becomes
35 # "col + 0" instead of just "col".
36- my ($real_col) = /^(\S+)/;
37+ my ($real_col) = /^UNIX_TIMESTAMP\((.+?)\)/;
38 $col .= " AS $real_col";
39 }
40 elsif ( $col =~ m/TRIM/ ) {
41@@ -216,7 +216,7 @@
42 my $type = $tbl_struct->{type_for}->{$_};
43 my $result = $q->quote($_);
44 if ( $type eq 'timestamp' ) {
45- $result .= ' + 0';
46+ $result = "UNIX_TIMESTAMP($result)";
47 }
48 elsif ( $float_precision && $type =~ m/float|double/ ) {
49 $result = "ROUND($result, $float_precision)";
50
51=== modified file 't/lib/RowChecksum.t'
52--- t/lib/RowChecksum.t 2012-10-30 15:38:31 +0000
53+++ t/lib/RowChecksum.t 2015-01-19 22:19:29 +0000
54@@ -126,11 +126,11 @@
55 tbl => $tbl,
56 func => 'SHA1',
57 ),
58- 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`, }
59+ 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`, }
60 . q{SHA1(CONCAT_WS('#', }
61 . q{`film_id`, `title`, `description`, `release_year`, `language_id`, }
62 . q{`original_language_id`, `rental_duration`, `rental_rate`, `length`, }
63- . q{`replacement_cost`, `rating`, `special_features`, `last_update` + 0, }
64+ . q{`replacement_cost`, `rating`, `special_features`, UNIX_TIMESTAMP(`last_update`), }
65 . q{CONCAT(ISNULL(`description`), ISNULL(`release_year`), }
66 . q{ISNULL(`original_language_id`), ISNULL(`length`), }
67 . q{ISNULL(`rating`), ISNULL(`special_features`))))},
68@@ -142,11 +142,11 @@
69 tbl => $tbl,
70 func => 'FNV_64',
71 ),
72- 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`, }
73+ 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`, }
74 . q{FNV_64(}
75 . q{`film_id`, `title`, `description`, `release_year`, `language_id`, }
76 . q{`original_language_id`, `rental_duration`, `rental_rate`, `length`, }
77- . q{`replacement_cost`, `rating`, `special_features`, `last_update` + 0)},
78+ . q{`replacement_cost`, `rating`, `special_features`, UNIX_TIMESTAMP(`last_update`))},
79 'FNV_64 query for sakila.film',
80 );
81
82
83=== modified file 't/pt-table-checksum/bugs.t'
84--- t/pt-table-checksum/bugs.t 2013-10-02 18:26:11 +0000
85+++ t/pt-table-checksum/bugs.t 2015-01-19 22:19:29 +0000
86@@ -295,6 +295,31 @@
87 );
88
89 # #############################################################################
90+# pt-table-checksum has errors when slaves have different system_time_zone
91+# https://bugs.launchpad.net/percona-toolkit/+bug/1388870
92+# #############################################################################
93+
94+# make slave set diferent system_time_zone by changing env var TZ.
95+diag(`/tmp/12346/stop >/dev/null`);
96+diag(`export TZ='HST';/tmp/12346/start >/dev/null`);
97+
98+$output = output(
99+ sub { pt_table_checksum::main(@args, qw(-t sakila.payment)) },
100+);
101+
102+
103+is(
104+ PerconaTest::count_checksum_results($output, 'diffs'),
105+ 0,
106+ "Bug 1388870 - No false positive reported when system_tz differ on slave"
107+);
108+
109+# restore slave to original system_tz
110+diag(`/tmp/12346/stop >/dev/null`);
111+diag(`/tmp/12346/start >/dev/null`);
112+
113+
114+# #############################################################################
115 # Done.
116 # #############################################################################
117 $sb->wipe_clean($master_dbh);
118
119=== modified file 't/pt-table-checksum/samples/chunkidx004.txt'
120--- t/pt-table-checksum/samples/chunkidx004.txt 2011-12-27 18:12:40 +0000
121+++ t/pt-table-checksum/samples/chunkidx004.txt 2015-01-19 22:19:29 +0000
122@@ -2,7 +2,7 @@
123 -- sakila.city
124 --
125
126-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*/
127+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*/
128
129 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*/
130
131
132=== modified file 't/pt-table-checksum/samples/chunkidx005.txt'
133--- t/pt-table-checksum/samples/chunkidx005.txt 2011-12-27 18:12:40 +0000
134+++ t/pt-table-checksum/samples/chunkidx005.txt 2015-01-19 22:19:29 +0000
135@@ -2,7 +2,7 @@
136 -- sakila.city
137 --
138
139-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*/
140+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*/
141
142 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*/
143
144
145=== modified file 't/pt-table-checksum/samples/n-chunk-index-cols.txt'
146--- t/pt-table-checksum/samples/n-chunk-index-cols.txt 2012-06-10 14:43:42 +0000
147+++ t/pt-table-checksum/samples/n-chunk-index-cols.txt 2015-01-19 22:19:29 +0000
148@@ -2,7 +2,7 @@
149 -- sakila.rental
150 --
151
152-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*/
153+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*/
154
155 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*/
156

Subscribers

People subscribed via source and target branches

to all changes: