Thread: pg_combinebackup PITR comparison test fix

Hi hackers,

While I was going through the TAP tests to fix the formatting of command
argument lists to be less confusing, I noticed that pg_combinebackup's
002_compare_backups.pl test accidentally ran pg_dumpall twice against
the same database, thus rendering the tests useless.

Fixing that revealed that there is a difference in the dumps: the
tablespaces have different paths.  pg_dumpall doesn't have an option to
map tablespace paths, so instead I used File::Compare::compare_text()
with a custom comparison function to erase the difference.

- ilmari

From d8f186e79fceb881a225c7b612e074d8956da1ec Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Sat, 14 Dec 2024 21:08:16 +0000
Subject: [PATCH] Fix pg_combinebackup PITR comparison test

The test was creating both the dumps to compare from the same DB, so
would never detect any mismatches.

Fixing that revealed that there is a difference in the dumps: the
tablespaces have different paths.  pg_dumpall doesn't have an option
to map tablespace paths, so use compare_text() with a custom
comparison function to erase the difference.
---
 .../pg_combinebackup/t/002_compare_backups.pl    | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/bin/pg_combinebackup/t/002_compare_backups.pl b/src/bin/pg_combinebackup/t/002_compare_backups.pl
index 63a0255de15..6bf48971734 100644
--- a/src/bin/pg_combinebackup/t/002_compare_backups.pl
+++ b/src/bin/pg_combinebackup/t/002_compare_backups.pl
@@ -2,7 +2,7 @@
 
 use strict;
 use warnings FATAL => 'all';
-use File::Compare;
+use File::Compare qw(compare_text);
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
@@ -175,17 +175,23 @@
         $pitr1->connstr('postgres'),
     ],
     'dump from PITR 1');
-$pitr1->command_ok(
+$pitr2->command_ok(
     [
         'pg_dumpall', '-f',
         $dump2, '--no-sync',
         '--no-unlogged-table-data', '-d',
-        $pitr1->connstr('postgres'),
+        $pitr2->connstr('postgres'),
     ],
     'dump from PITR 2');
 
-# Compare the two dumps, there should be no differences.
-my $compare_res = compare($dump1, $dump2);
+# Compare the two dumps, there should be no differences other than
+# the tablespace paths.
+my $compare_res = compare_text(
+    $dump1, $dump2,
+    sub {
+        s{create tablespace .* location '.*/tspitr\K[12]}{N}i for @_;
+        return $_[0] ne $_[1];
+    });
 note($dump1);
 note($dump2);
 is($compare_res, 0, "dumps are identical");
-- 
2.39.5

Michael Paquier <michael@paquier.xyz> writes:

> On Sun, Dec 15, 2024 at 10:34:07AM +0900, Michael Paquier wrote:
>> Indeed, good catch.  I'll take care of it.

Thanks!

> +    sub {
> +        s{create tablespace .* location '.*/tspitr\K[12]}{N}i for @_;
> +        return $_[0] ne $_[1];
> +    });
>
> The CI is complaining on this one because the custom comparison
> function is not able to digest WIN32 paths, leading to failures in the
> dump comparison like that:
> -CREATE TABLESPACE ts1 OWNER "SYSTEM" LOCATION
> E'C:\\Windows\\TEMP\\tJ4qTmrkZv\\tspitr1';
> +CREATE TABLESPACE ts1 OWNER "SYSTEM" LOCATION
> E'C:\\Windows\\TEMP\\tJ4qTmrkZv\\tspitr2';
>
> So there is an issue with the slash character after the location and
> the single space before the quote.  We could use something like this
> one which would handle the paths sanely:
> s{create tablespace .* location .*'.*tspitr\K[12]}{N}i for @_;
>
> Perhaps you are able to come with a more elegant string?

I'm torn between making it more specific, only allowing escaped strings
or not, and either type of slash for the path separator:

s{create tablespace .* location .* E?'.*[\\/]tspitr\K[12]}{N}i for @_;

vs. making it less specific, not caring about the specifics of quoting
or path separators at all:

s{create tablespace .* location .*\btspitr\K[12]}{N}i for @_;

I think I'm leaning towards the latter, for simplicity and robustness.

- ilmari

Michael Paquier <michael@paquier.xyz> writes:

> On Mon, Dec 16, 2024 at 12:26:38PM +0000, Dagfinn Ilmari Mannsåker wrote:
>> s{create tablespace .* location .*\btspitr\K[12]}{N}i for @_;
>> 
>> I think I'm leaning towards the latter, for simplicity and robustness.
>
> Simplicity and robustness works here and in the CI, so I have used the
> latter then applied your patch.  Thanks!

Thanks!

- ilmari