Horton

Thread

Commits

  1. pg_combinebackup: Fix PITR comparison test in 002_compare_backups

  1. pg_combinebackup PITR comparison test fix

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-12-14T21:57:11Z

    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

  2. Re: pg_combinebackup PITR comparison test fix

    Michael Paquier <michael@paquier.xyz> — 2024-12-15T01:34:07Z

    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

  3. Re: pg_combinebackup PITR comparison test fix

    Michael Paquier <michael@paquier.xyz> — 2024-12-16T05:14:55Z

    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

  4. Re: pg_combinebackup PITR comparison test fix

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-12-16T12:26:38Z

    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

  5. Re: pg_combinebackup PITR comparison test fix

    Michael Paquier <michael@paquier.xyz> — 2024-12-17T00:25:38Z

    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

  6. Re: pg_combinebackup PITR comparison test fix

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-12-17T10:47:27Z

    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