Fix for 886059: pt-heartbeat handles timezones inconsistently

bin/pt-heartbeat

@@ -4734,7 +4734,7 @@ sub main {
       $dbh->do($sql);
 
       $sql = ($o->get('replace') ? "REPLACE" : "INSERT")
-           . qq/ INTO $db_tbl (ts, server_id) VALUES (NOW(), $server_id)/;
+           . qq/ INTO $db_tbl (ts, server_id) VALUES (UTC_TIMESTAMP(), $server_id)/;
       PTDEBUG && _d($sql);
       # This may fail if the table already existed and already had this row.
       # We eval to ignore this possibility.
@@ -4831,7 +4831,7 @@ sub main {
          PTDEBUG && _d('No heartbeat row in table');
          if ( $o->get('insert-heartbeat-row') ) {
             my $sql = "INSERT INTO $db_tbl ($pk_col, ts) "
-                    . "VALUES ('$pk_val', NOW())";
+                    . "VALUES ('$pk_val', UTC_TIMESTAMP())";
             PTDEBUG && _d($sql);
             $dbh->do($sql);
          }
@@ -4920,7 +4920,8 @@ sub main {
             }
          }
 
-         $sth->execute(ts(time), @vals);
+         my ($now) = $dbh->selectrow_array(q{SELECT UTC_TIMESTAMP()});
+         $sth->execute($now, @vals);
          PTDEBUG && _d($sth->{Statement});
          $sth->finish();
 
@@ -4930,8 +4931,12 @@ sub main {
    else { # --monitor or --check
       my $dbi_driver = lc $o->get('dbi-driver');
 
+      # UNIX_TIMESTAMP(UTC_TIMESTAMP()) instead of UNIX_TIMESTAMP() alone,
+      # so we make sure that we aren't being fooled by a timezone.
+      # UNIX_TIMESTAMP(ts) replaces unix_timestamp($ts) -- MySQL is the
+      # authority here, so let it calculate everything.
       $heartbeat_sql
-         = "SELECT ts"
+         = "SELECT UNIX_TIMESTAMP(UTC_TIMESTAMP()), UNIX_TIMESTAMP(ts)"
          . ($dbi_driver eq 'mysql' ? '/*!50038, @@hostname AS host*/' : '')
          . ($id ? "" : ", server_id")
          . " FROM $db_tbl "
@@ -4945,13 +4950,12 @@ sub main {
          my ($sth) = @_;
          $sth->execute();
          PTDEBUG && _d($sth->{Statement});
-         my ($ts, $hostname, $server_id) = $sth->fetchrow_array();
+         my ($now, $ts, $hostname, $server_id) = $sth->fetchrow_array();
-         my $now = time;
          PTDEBUG && _d("Heartbeat from server", $server_id, "\n",
-            " now:", ts($now), "\n",
+            " now:", $now, "\n",
             "  ts:", $ts, "\n",
             "skew:", $skew);
-         my $delay = $now - unix_timestamp($ts) - $skew;
+         my $delay = $now - $ts - $skew;
          PTDEBUG && _d('Delay', sprintf('%.6f', $delay), 'on', $hostname);
 
          # Because we adjust for skew, if the ts are less than skew seconds
@@ -5446,7 +5450,7 @@ be created with the following MAGIC_create_heartbeat table definition:
 The heartbeat table requires at least one row.  If you manually create the
 heartbeat table, then you must insert a row by doing:
 
-  INSERT INTO heartbeat (ts, server_id) VALUES (NOW(), N);
+  INSERT INTO heartbeat (ts, server_id) VALUES (UTC_TIMESTAMP(), N);
 
 where C<N> is the server's ID; do not use @@server_id because it will replicate
 and slaves will insert their own server ID instead of the master's server ID.
@@ -5464,7 +5468,7 @@ Legacy tables do not support L<"--update"> instances on each slave
 of a multi-slave hierarchy like "master -> slave1 -> slave2".
 To manually insert the one required row into a legacy table:
 
-  INSERT INTO heartbeat (id, ts) VALUES (1, NOW());
+  INSERT INTO heartbeat (id, ts) VALUES (1, UTC_TIMESTAMP());
 
 The tool automatically detects if the heartbeat table is legacy.
 

t/pt-heartbeat/bugs.t

@@ -0,0 +1,108 @@
+#!/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 POSIX qw( tzset );
+use File::Temp qw(tempfile);
+
+use PerconaTest;
+use Sandbox;
+require "$trunk/bin/pt-heartbeat";
+
+my $dp  = new DSNParser(opts=>$dsn_opts);
+my $sb  = new Sandbox(basedir => '/tmp', DSNParser => $dp);
+my $master_dbh = $sb->get_dbh_for('master');
+my $slave1_dbh = $sb->get_dbh_for('slave1');
+my $slave2_dbh = $sb->get_dbh_for('slave2');
+
+if ( !$master_dbh ) {
+   plan skip_all => 'Cannot connect to sandbox master';
+}
+elsif ( !$slave1_dbh ) {
+   plan skip_all => 'Cannot connect to sandbox slave1';
+}
+elsif ( !$slave2_dbh ) {
+   plan skip_all => 'Cannot connect to sandbox slave2';
+}
+
+unlink '/tmp/pt-heartbeat-sentinel';
+$sb->create_dbs($master_dbh, ['test']);
+$sb->wait_for_slaves();
+
+my $output;
+my $base_pidfile = (tempfile("/tmp/pt-heartbeat-test.XXXXXXXX", OPEN => 0, UNLINK => 0))[1];
+my $master_port = $sb->port_for('master');
+
+my @exec_pids;
+my @pidfiles;
+
+sub start_update_instance {
+   my ($port) = @_;
+   my $pidfile = "$base_pidfile.$port.pid";
+   push @pidfiles, $pidfile;
+
+   my $pid = fork();
+   if ( $pid == 0 ) {
+      my $cmd = "$trunk/bin/pt-heartbeat";
+      exec { $cmd } $cmd, qw(-h 127.0.0.1 -u msandbox -p msandbox -P), $port,
+                          qw(--database test --table heartbeat --create-table),
+                          qw(--update --interval 0.5 --pid), $pidfile;
+      exit 1;
+   }
+   push @exec_pids, $pid;
+   
+   PerconaTest::wait_for_files($pidfile);
+   ok(
+      -f $pidfile,
+      "--update on $port started"
+   );
+}
+
+sub stop_all_instances {
+   my @pids = @exec_pids, map { chomp; $_ } map { slurp_file($_) } @pidfiles;
+   diag(`$trunk/bin/pt-heartbeat --stop >/dev/null`);
+
+   waitpid($_, 0) for @pids;
+   PerconaTest::wait_until(sub{ !-e $_ }) for @pidfiles;
+}
+
+# ############################################################################
+# pt-heartbeat handles timezones inconsistently
+# https://bugs.launchpad.net/percona-toolkit/+bug/886059
+# ############################################################################
+
+start_update_instance( $master_port );
+
+my $slave1_dsn = $sb->dsn_for('slave1');
+# Using full_output here to work around a Perl bug: Only the first explicit
+# tzset works.
+($output) = full_output(sub {
+   local $ENV{TZ} = '-09:00';
+   tzset();
+   pt_heartbeat::main($slave1_dsn, qw(--database test --table heartbeat),
+                        qw(--check --master-server-id), $master_port)
+});
+
+like(
+   $output,
+   qr/\A\d.\d{2}$/,
+   "Bug 886059: pt-heartbeat doesn't get confused with differing timezones"
+);
+
+stop_all_instances();
+
+# ############################################################################
+# Done.
+# ############################################################################
+$sb->wipe_clean($master_dbh);
+ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox");
+done_testing;