pg_combinebackup PITR comparison test fix

On Sat, Dec 14, 2024 at 09:57:11PM +0000, Dagfinn Ilmari Mannsåker wrote:

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.

Indeed, good catch. I'll take care of it.
--
Michael

On Sun, Dec 15, 2024 at 10:34:07AM +0900, Michael Paquier wrote:

Indeed, good catch. I'll take care of it.

+    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?
--
Michael

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

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!
--
Michael

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