PT-2378 - extended FP precision in pt-table-sync

pt-table-sync now uses up to 17 decimal digits when writing floating point numbers in the generated SQL statements. This is necessary to prevent unintended data changes.
percona · Sep 15, 2024 · 6cbe827 · 6cbe827
1 parent 206404f
commit 6cbe827
Show file tree

Hide file tree

Showing 19 changed files with 145 additions and 16 deletions.
diff --git a/bin/pt-archiver b/bin/pt-archiver
@@ -2993,7 +2993,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-deadlock-logger b/bin/pt-deadlock-logger
@@ -2088,7 +2088,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-duplicate-key-checker b/bin/pt-duplicate-key-checker
@@ -133,7 +133,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-find b/bin/pt-find
@@ -1690,7 +1690,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-fk-error-logger b/bin/pt-fk-error-logger
@@ -1242,7 +1242,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-heartbeat b/bin/pt-heartbeat
@@ -3492,7 +3492,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-index-usage b/bin/pt-index-usage
@@ -589,7 +589,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-kill b/bin/pt-kill
@@ -4833,7 +4833,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-online-schema-change b/bin/pt-online-schema-change
@@ -2866,7 +2866,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-query-digest b/bin/pt-query-digest
@@ -1257,7 +1257,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-slave-restart b/bin/pt-slave-restart
@@ -135,7 +135,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-table-checksum b/bin/pt-table-checksum
@@ -4134,7 +4134,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-table-sync b/bin/pt-table-sync
@@ -1909,7 +1909,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/bin/pt-upgrade b/bin/pt-upgrade
@@ -1254,7 +1254,7 @@ sub quote_val {
    return $val if $val =~ m/^0x[0-9a-fA-F]+$/  # quote hex data
                   && !$args{is_char};          # unless is_char is true
 
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    $val =~ s/(['\\])/\\$1/g;
    return "'$val'";

diff --git a/lib/Quoter.pm b/lib/Quoter.pm
@@ -78,7 +78,7 @@ sub quote_val {
                   && !$args{is_char};          # unless is_char is true
 
    # https://bugs.launchpad.net/percona-toolkit/+bug/1229861
-   return $val if $args{is_float};
+   return sprintf("%.17g", $val) if $args{is_float};
 
    # Quote and return non-numeric vals.
    $val =~ s/(['\\])/\\$1/g;

diff --git a/t/lib/Quoter.t b/t/lib/Quoter.t
@@ -70,6 +70,7 @@ is( $q->quote_val('0x89504E470', is_char => 0), '0x89504E470', 'hex string, with
 is( $q->quote_val('0x89504E470', is_char => 1), "'0x89504E470'", 'hex string, with is_char => 1');
 is( $q->quote_val('0x89504I470'), "'0x89504I470'", 'looks like hex string');
 is( $q->quote_val('eastside0x3'), "'eastside0x3'", 'looks like hex str (issue 1110');
+is( $q->quote_val(969.1 / 360, is_float => 1), "2.6919444444444447", 'float has full precision');
 
 # Splitting DB and tbl apart
 is_deeply(

diff --git a/t/pt-table-sync/float_precision.t b/t/pt-table-sync/float_precision.t
@@ -48,7 +48,7 @@ $output = `$trunk/bin/pt-table-sync --sync-to-master h=127.1,P=12346,u=msandbox,
 $output = remove_traces($output);
 is(
    $output,
-   "REPLACE INTO `test`.`fl`(`id`, `f`, `d`) VALUES ('1', 1.0000011921, 2.0000012);
+   "REPLACE INTO `test`.`fl`(`id`, `f`, `d`) VALUES ('1', 1.0000011921, 2.0000011999999998);
 ",
    'No --float-precision so double col diff at high precision (issue 410)'
 );
@@ -63,6 +63,23 @@ is(
    '--float-precision so no more diff (issue 410)'
 );
 
+# Although the SQL statement contains serialized values with more than necessary decimal digits
+# we produce the expected value on execution
+$output = `$trunk/bin/pt-table-sync --sync-to-master h=127.1,P=12346,u=msandbox,p=msandbox,D=test,t=fl --execute 2>&1`;
+is(
+   $output,
+   '',
+   'REPLACE statement can be successfully applied'
+);
+
+$sb->wait_for_slaves();
+my @rows = $slave_dbh->selectrow_array('SELECT `d` FROM `test`.`fl` WHERE `d` = 2.0000012');
+is_deeply(
+   \@rows,
+   [2.0000012],
+   'Floating point values are set correctly in round trip'
+);
+
 # #############################################################################
 # pt-table-sync quotes floats, prevents syncing
 # https://bugs.launchpad.net/percona-toolkit/+bug/1229861

diff --git a/t/pt-table-sync/pt-2378.t b/t/pt-table-sync/pt-2378.t
@@ -0,0 +1,96 @@
+#!/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 PerconaTest;
+use Sandbox;
+require "$trunk/bin/pt-table-sync";
+
+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');
+
+if ( !$master_dbh ) {
+   plan skip_all => 'Cannot connect to sandbox master';
+}
+elsif ( !$slave1_dbh ) {
+   plan skip_all => 'Cannot connect to sandbox slave1';
+}
+else {
+   plan tests => 5;
+}
+
+my ($output, @rows);
+
+# #############################################################################
+# Test generated REPLACE statements.
+# #############################################################################
+$sb->load_file('master', "t/pt-table-sync/samples/pt-2378.sql");
+$sb->wait_for_slaves();
+$slave1_dbh->do("update `test`.`test_table` set `some_string` = 'c' where `id` = 1");
+
+$output = remove_traces(output(
+   sub { pt_table_sync::main('--sync-to-master',
+      'h=127.0.0.1,P=12346,u=msandbox,p=msandbox',
+      qw(-t test.test_table --print --execute))
+   },
+));
+chomp($output);
+is(
+   $output,
+   "REPLACE INTO `test`.`test_table`(`id`, `value1`, `value2`, `some_string`) VALUES ('1', 315.25999999999942, 2.6919444444444447, 'a');",
+   "Floating point numbers are generated with sufficient precision in REPLACE statements"
+);
+
+$sb->wait_for_slaves();
+my $query = 'SELECT * FROM `test`.`test_table` WHERE `value1` = 315.2599999999994 AND `value2` = 2.6919444444444447';
+@rows = $slave1_dbh->selectrow_array($query);
+is_deeply(
+   \@rows,
+   [1, 315.2599999999994, 2.6919444444444447, 'a'],
+   'Floating point values are set correctly in round trip'
+);
+
+# #############################################################################
+# Test generated UPDATE statements.
+# #############################################################################
+$sb->load_file('master', "t/pt-table-sync/samples/pt-2378.sql");
+$sb->wait_for_slaves();
+$slave1_dbh->do("update `test`.`test_table` set `some_string` = 'c' where `id` = 1");
+
+$output = remove_traces(output(
+   sub { pt_table_sync::main(qw(--print --execute),
+      "h=127.0.0.1,P=12346,u=msandbox,p=msandbox,D=test,t=test_table",
+      "h=127.0.0.1,P=12345,u=msandbox,p=msandbox,D=test,t=test_table");
+   }
+));
+chomp($output);
+is(
+   $output,
+   "UPDATE `test`.`test_table` SET `value1`=315.25999999999942, `value2`=2.6919444444444447, `some_string`='c' WHERE `id`='1' LIMIT 1;",
+   "Floating point numbers are generated with sufficient precision in UPDATE statements"
+);
+
+@rows = $master_dbh->selectrow_array($query);
+is_deeply(
+   \@rows,
+   [1, 315.2599999999994, 2.6919444444444447, 'c'],
+   'Floating point values are set correctly in round trip'
+);
+
+# #############################################################################
+# Done.
+# #############################################################################
+$sb->wipe_clean($master_dbh);
+ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox");
+exit;
diff --git a/t/pt-table-sync/samples/pt-2378.sql b/t/pt-table-sync/samples/pt-2378.sql
@@ -0,0 +1,15 @@
+DROP DATABASE IF EXISTS test;
+CREATE DATABASE test;
+USE test;
+
+CREATE TABLE `test_table` (
+  `id` BIGINT AUTO_INCREMENT PRIMARY KEY,
+  `value1` DOUBLE NOT NULL,
+  `value2` DOUBLE NOT NULL,
+  `some_string` VARCHAR(32) NOT NULL
+) ENGINE=InnoDB;
+
+INSERT INTO `test_table`
+ (`value1`, `value2`, `some_string`)
+VALUES
+ (315.2599999999994, 2.6919444444444447, 'a');