Thread: Test to dump and restore objects left behind by regression

Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

21 February 2024, 06:48:45

Hi All,
In [1] we found that having a test to dump and restore objects left
behind by regression test is missing. Such a test would cover many
dump restore scenarios without much effort. It will also help identity
problems described in the same thread [2] during development itself.

I am starting a new thread to discuss such a test. Attached is a WIP
version of the test. The test does fail at the restore step when
commit 74563f6b90216180fc13649725179fc119dddeb5 is reverted
reintroducing the problem.

Attached WIP test is inspired from
src/bin/pg_upgrade/t/002_pg_upgrade.pl which tests binary-upgrade
dumps. Attached test tests the non-binary-upgrade dumps.

Similar to 0002_pg_upgrade.pl the test uses SQL dumps before and after
dump and restore to make sure that the objects are restored correctly.
The test has some shortcomings
1. Objects which are not dumped at all are never tested.
2. Since the rows are dumped in varying order by the two clusters, the
test only tests schema dump and restore.
3. The order of columns of the inheritance child table differs
depending upon the DDLs used to reach a given state. This introduces
diffs in the SQL dumps before and after restore. The test ignores
these diffs by hardcoding the diff in the test.

Even with 1 and 2 the test is useful to detect dump/restore anomalies.
I think we should improve 3, but I don't have a good and simpler
solution. I didn't find any way to compare two given clusters in our
TAP test framework. Building it will be a lot of work. Not sure if
it's worth it.

Suggestions welcome.

[1] https://www.postgresql.org/message-id/CAExHW5vyqv%3DXLTcNMzCNccOrHiun_XhYPjcRqeV6dLvZSamriQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/3462358.1708107856%40sss.pgh.pa.us

--
Best Wishes,
Ashutosh Bapat

Attachment

0001-WIP-Test-to-dump-and-restore-object-left-be-20240221.patch

Re: Test to dump and restore objects left behind by regression

From

Michael Paquier

Date:

22 February 2024, 01:01:55

On Wed, Feb 21, 2024 at 12:18:45PM +0530, Ashutosh Bapat wrote:
> Even with 1 and 2 the test is useful to detect dump/restore anomalies.
> I think we should improve 3, but I don't have a good and simpler
> solution. I didn't find any way to compare two given clusters in our
> TAP test framework. Building it will be a lot of work. Not sure if
> it's worth it.

+    my $rc =
+      system($ENV{PG_REGRESS}
+          . " $extra_opts "
+          . "--dlpath=\"$dlpath\" "
+          . "--bindir= "
+          . "--host="
+          . $node->host . " "
+          . "--port="
+          . $node->port . " "
+          . "--schedule=$srcdir/src/test/regress/parallel_schedule "
+          . "--max-concurrent-tests=20 "
+          . "--inputdir=\"$inputdir\" "
+          . "--outputdir=\"$outputdir\"");

I am not sure that it is a good idea to add a full regression test
cycle while we have already 027_stream_regress.pl that would be enough
to test some dump scenarios.  These are very expensive and easy to
notice even with a high level of parallelization of the tests.
--
Michael

Attachment

signature.asc

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

22 February 2024, 08:53:05

On Thu, Feb 22, 2024 at 6:32 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Feb 21, 2024 at 12:18:45PM +0530, Ashutosh Bapat wrote:
> > Even with 1 and 2 the test is useful to detect dump/restore anomalies.
> > I think we should improve 3, but I don't have a good and simpler
> > solution. I didn't find any way to compare two given clusters in our
> > TAP test framework. Building it will be a lot of work. Not sure if
> > it's worth it.
>
> +       my $rc =
> +         system($ENV{PG_REGRESS}
> +                 . " $extra_opts "
> +                 . "--dlpath=\"$dlpath\" "
> +                 . "--bindir= "
> +                 . "--host="
> +                 . $node->host . " "
> +                 . "--port="
> +                 . $node->port . " "
> +                 . "--schedule=$srcdir/src/test/regress/parallel_schedule "
> +                 . "--max-concurrent-tests=20 "
> +                 . "--inputdir=\"$inputdir\" "
> +                 . "--outputdir=\"$outputdir\"");
>
> I am not sure that it is a good idea to add a full regression test
> cycle while we have already 027_stream_regress.pl that would be enough
> to test some dump scenarios.

That test *uses* pg_dump as a way to test whether the two clusters are
in sync. The test might change in future to use some other method to
make sure the two clusters are consistent. Adding the test here to
that test will make that change much harder.

It's not the dump, but restore, we are interested in here. No test
that runs PG_REGRESS also runs pg_restore in non-binary mode.

Also we need to keep this test near other pg_dump tests, not far from them.

> These are very expensive and easy to
> notice even with a high level of parallelization of the tests.

I agree, but I didn't find a suitable test to ride on.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Peter Eisentraut

Date:

22 February 2024, 09:16:50

On 22.02.24 02:01, Michael Paquier wrote:
> On Wed, Feb 21, 2024 at 12:18:45PM +0530, Ashutosh Bapat wrote:
>> Even with 1 and 2 the test is useful to detect dump/restore anomalies.
>> I think we should improve 3, but I don't have a good and simpler
>> solution. I didn't find any way to compare two given clusters in our
>> TAP test framework. Building it will be a lot of work. Not sure if
>> it's worth it.
> 
> +    my $rc =
> +      system($ENV{PG_REGRESS}
> +          . " $extra_opts "
> +          . "--dlpath=\"$dlpath\" "
> +          . "--bindir= "
> +          . "--host="
> +          . $node->host . " "
> +          . "--port="
> +          . $node->port . " "
> +          . "--schedule=$srcdir/src/test/regress/parallel_schedule "
> +          . "--max-concurrent-tests=20 "
> +          . "--inputdir=\"$inputdir\" "
> +          . "--outputdir=\"$outputdir\"");
> 
> I am not sure that it is a good idea to add a full regression test
> cycle while we have already 027_stream_regress.pl that would be enough
> to test some dump scenarios.  These are very expensive and easy to
> notice even with a high level of parallelization of the tests.

The problem is, we don't really have any end-to-end coverage of

dump
restore
dump again
compare the two dumps

with a database with lots of interesting objects in it.

Note that each of these steps could fail.

We have somewhat relied on the pg_upgrade test to provide this testing, 
but we have recently discovered that the dumps in binary-upgrade mode 
are different enough to not test the normal dumps well.

Yes, this test is a bit expensive.  We could save some time by doing the 
first dump at the end of the normal regress test and have the pg_dump 
test reuse that, but then that would make the regress test run a bit 
longer.  Is that a better tradeoff?

I have done some timing tests:

master:

pg_dump check:     22s
pg_dump check -j8: 8s
check-world -j8:   2min44s

patched:

pg_dump check:     34s
pg_dump check -j8: 13s
check-world -j8:   2min46s

So overall it doesn't seem that bad.

Re: Test to dump and restore objects left behind by regression

From

Daniel Gustafsson

Date:

22 February 2024, 09:33:04

> On 22 Feb 2024, at 10:16, Peter Eisentraut <peter@eisentraut.org> wrote:

> We have somewhat relied on the pg_upgrade test to provide this testing, but we have recently discovered that the
dumpsin binary-upgrade mode are different enough to not test the normal dumps well. 
>
> Yes, this test is a bit expensive.  We could save some time by doing the first dump at the end of the normal regress
testand have the pg_dump test reuse that, but then that would make the regress test run a bit longer.  Is that a better
tradeoff?

Something this expensive seems like what PG_TEST_EXTRA is intended for, we
already have important test suites there.

But.  We know that the cluster has an interesting state when the pg_upgrade
test starts, could we use that to make a dump/restore test before continuing
with testing pg_upgrade?  It can be argued that pg_upgrade shouldn't be
responsible for testing pg_dump, but it's already now a pretty important
testcase for pg_dump in binary upgrade mode so it's that far off.  If pg_dump
has bugs then pg_upgrade risks subtly breaking.

When upgrading to the same version, we could perhaps also use this to test a
scenario like: Dump A, restore into B, upgrade B into C, dump C and compare C
to A.

--
Daniel Gustafsson

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

22 February 2024, 09:55:42

On Thu, Feb 22, 2024 at 3:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 22 Feb 2024, at 10:16, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> > We have somewhat relied on the pg_upgrade test to provide this testing, but we have recently discovered that the
dumpsin binary-upgrade mode are different enough to not test the normal dumps well. 
> >
> > Yes, this test is a bit expensive.  We could save some time by doing the first dump at the end of the normal
regresstest and have the pg_dump test reuse that, but then that would make the regress test run a bit longer.  Is that
abetter tradeoff? 
>
> Something this expensive seems like what PG_TEST_EXTRA is intended for, we
> already have important test suites there.

That's ok with me.

>
> But.  We know that the cluster has an interesting state when the pg_upgrade
> test starts, could we use that to make a dump/restore test before continuing
> with testing pg_upgrade?  It can be argued that pg_upgrade shouldn't be
> responsible for testing pg_dump, but it's already now a pretty important
> testcase for pg_dump in binary upgrade mode so it's that far off.  If pg_dump
> has bugs then pg_upgrade risks subtly breaking.

Somebody looking for dump/restore tests wouldn't search
src/bin/pg_upgrade, I think. However if more people think we should
just add this test 002_pg_upgrade.pl, I am fine with it.

>
> When upgrading to the same version, we could perhaps also use this to test a
> scenario like: Dump A, restore into B, upgrade B into C, dump C and compare C
> to A.

If comparison of C to A fails, we wouldn't know which step fails. I
would rather compare outputs of each step separately.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Daniel Gustafsson

Date:

22 February 2024, 10:00:58

> On 22 Feb 2024, at 10:55, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> On Thu, Feb 22, 2024 at 3:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:

> Somebody looking for dump/restore tests wouldn't search
> src/bin/pg_upgrade, I think.

Quite possibly not, but pg_upgrade is already today an important testsuite for
testing pg_dump in binary-upgrade mode so maybe more developers touching
pg_dump should?

>> When upgrading to the same version, we could perhaps also use this to test a
>> scenario like: Dump A, restore into B, upgrade B into C, dump C and compare C
>> to A.
>
> If comparison of C to A fails, we wouldn't know which step fails. I
> would rather compare outputs of each step separately.

To be clear, this wasn't intended to replace what you are proposing, but an
idea for using it to test *more* scenarios.

--
Daniel Gustafsson

Re: Test to dump and restore objects left behind by regression

From

Peter Eisentraut

Date:

22 February 2024, 10:20:29

On 22.02.24 11:00, Daniel Gustafsson wrote:
>> On 22 Feb 2024, at 10:55, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>> On Thu, Feb 22, 2024 at 3:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> Somebody looking for dump/restore tests wouldn't search
>> src/bin/pg_upgrade, I think.
> 
> Quite possibly not, but pg_upgrade is already today an important testsuite for
> testing pg_dump in binary-upgrade mode so maybe more developers touching
> pg_dump should?

Yeah, I think attaching this to the existing pg_upgrade test would be a 
good idea.  Not only would it save test run time, it would probably also 
reduce code duplication.

Re: Test to dump and restore objects left behind by regression

From

Tom Lane

Date:

22 February 2024, 15:05:48

Peter Eisentraut <peter@eisentraut.org> writes:
> The problem is, we don't really have any end-to-end coverage of

> dump
> restore
> dump again
> compare the two dumps

> with a database with lots of interesting objects in it.

I'm very much against adding another full run of the core regression
tests to support this.  But beyond the problem of not bloating the
check-world test runtime, there is the question of what this would
actually buy us.  I doubt that it is worth very much, because
it would not detect bugs-of-omission in pg_dump.  As I remarked in
the other thread, if pg_dump is blind to the existence of some
feature or field, testing that the dumps compare equal will fail
to reveal that it didn't restore that property.

I'm not sure what we could do about that.  One could imagine writing
some test infrastructure that dumps out the contents of the system
catalogs directly, and comparing that instead of pg_dump output.
But that'd be a lot of infrastructure to write and maintain ...
and it's not real clear why it wouldn't *also* suffer from
I-forgot-to-add-this hazards.

On balance, I think there are good reasons that we've not added
such a test, and I don't believe those tradeoffs have changed.

            regards, tom lane

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

23 February 2024, 05:03:42

On Thu, Feb 22, 2024 at 3:50 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 22.02.24 11:00, Daniel Gustafsson wrote:
> >> On 22 Feb 2024, at 10:55, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> >> On Thu, Feb 22, 2024 at 3:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> >
> >> Somebody looking for dump/restore tests wouldn't search
> >> src/bin/pg_upgrade, I think.
> >
> > Quite possibly not, but pg_upgrade is already today an important testsuite for
> > testing pg_dump in binary-upgrade mode so maybe more developers touching
> > pg_dump should?
>
> Yeah, I think attaching this to the existing pg_upgrade test would be a
> good idea.  Not only would it save test run time, it would probably also
> reduce code duplication.
>

That's more than one vote for adding the test to 002_pg_ugprade.pl.
Seems fine to me.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

23 February 2024, 05:16:01

On Thu, Feb 22, 2024 at 8:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Eisentraut <peter@eisentraut.org> writes:
> > The problem is, we don't really have any end-to-end coverage of
>
> > dump
> > restore
> > dump again
> > compare the two dumps
>
> > with a database with lots of interesting objects in it.
>
> I'm very much against adding another full run of the core regression
> tests to support this.

This will be taken care of by Peter's latest idea of augmenting
existing 002_pg_upgrade.pl.

> But beyond the problem of not bloating the
> check-world test runtime, there is the question of what this would
> actually buy us.  I doubt that it is worth very much, because
> it would not detect bugs-of-omission in pg_dump.  As I remarked in
> the other thread, if pg_dump is blind to the existence of some
> feature or field, testing that the dumps compare equal will fail
> to reveal that it didn't restore that property.
>
> I'm not sure what we could do about that.  One could imagine writing
> some test infrastructure that dumps out the contents of the system
> catalogs directly, and comparing that instead of pg_dump output.
> But that'd be a lot of infrastructure to write and maintain ...
> and it's not real clear why it wouldn't *also* suffer from
> I-forgot-to-add-this hazards.

If a developer forgets to add logic to dump objects that their patch
adds, it's hard to detect it, through testing alone, in every possible
case. We need reviewers to take care of that. I don't think that's the
objective of this test case or of pg_upgrade test either.

>
> On balance, I think there are good reasons that we've not added
> such a test, and I don't believe those tradeoffs have changed.
>

I am not aware of those reasons. Are they documented somewhere? Any
pointers to the previous discussion on this topic? Googling "pg_dump
regression pgsql-hackers" returns threads about performance
regressions.

On the flip side, the test I wrote reproduces the COMPRESSION/STORAGE
bug you reported along with a few other bugs in that area which I will
report soon on that thread. I think, that shows that we need such a
test.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

26 April 2024, 13:08:22

On Fri, Feb 23, 2024 at 10:46 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:

On Thu, Feb 22, 2024 at 8:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Eisentraut <peter@eisentraut.org> writes:
> > The problem is, we don't really have any end-to-end coverage of
>
> > dump
> > restore
> > dump again
> > compare the two dumps
>
> > with a database with lots of interesting objects in it.
>
> I'm very much against adding another full run of the core regression
> tests to support this.

This will be taken care of by Peter's latest idea of augmenting
existing 002_pg_upgrade.pl.

Incorporated the test to 002_pg_ugprade.pl.

Some points for discussion:

1. The test still hardcodes the diffs between two dumps. Haven't found a better way to do it. I did consider removing the problematic objects from the regression database but thought against it since we would lose some coverage.

2. The new code tests dump and restore of just the regression database and does not use pg_dumpall like pg_upgrade. Should it instead perform pg_dumpall? I decided against it since a. we are interested in dumping and restoring objects left behind by regression, b. I didn't find a way to provide the format option to pg_dumpall. The test could be enhanced to use different dump formats.

I have added it to the next commitfest. https://commitfest.postgresql.org/48/4956/

Best Wishes,

Ashutosh Bapat

Attachment

0001-pg_dump-restore-regression-objects-20240426.patch

Re: Test to dump and restore objects left behind by regression

From

Michael Paquier

Date:

03 June 2024, 22:58:27

On Fri, Apr 26, 2024 at 06:38:22PM +0530, Ashutosh Bapat wrote:
> Some points for discussion:
> 1. The test still hardcodes the diffs between two dumps. Haven't found a
> better way to do it. I did consider removing the problematic objects from
> the regression database but thought against it since we would lose some
> coverage.
>
> 2. The new code tests dump and restore of just the regression database and
> does not use pg_dumpall like pg_upgrade. Should it instead perform
> pg_dumpall? I decided against it since a. we are interested in dumping and
> restoring objects left behind by regression, b. I didn't find a way to
> provide the format option to pg_dumpall. The test could be enhanced to use
> different dump formats.
>
> I have added it to the next commitfest.
> https://commitfest.postgresql.org/48/4956/

Ashutosh and I have discussed this patch a bit last week.  Here is a
short summary of my input, after I understood what is going on.

+    # We could avoid this by dumping the database loaded from original dump.
+    # But that would change the state of the objects as left behind by the
+    # regression.
+    my $expected_diff = " --
+ CREATE TABLE public.gtestxx_4 (
+-    b integer,
+-    a integer NOT NULL
++    a integer NOT NULL,
++    b integer
+ )
[...]
+    my ($stdout, $stderr) =
+        run_command([ 'diff', '-u', $dump4_file, $dump5_file]);
+    # Clear file names, line numbers from the diffs; those are not going to
+    # remain the same always. Also clear empty lines and normalize new line
+    # characters across platforms.
+    $stdout =~ s/^\@\@.*$//mg;
+    $stdout =~ s/^.*$dump4_file.*$//mg;
+    $stdout =~ s/^.*$dump5_file.*$//mg;
+    $stdout =~ s/^\s*\n//mg;
+    $stdout =~ s/\r\n/\n/g;
+    $expected_diff =~ s/\r\n/\n/g;
+    is($stdout, $expected_diff, 'old and new dumps match after dump and restore');
+}

I am not a fan of what this patch does, adding the knowledge related
to the dump filtering within 002_pg_upgrade.pl.  Please do not take
me wrong, I am not against the idea of adding that within this
pg_upgrade test to save from one full cycle of `make check` to check
the consistency of the dump.  My issue is that this logic should be
externalized, and it should be in fewer lines of code.

For the externalization part, Ashutosh and I considered a few ideas,
but one that we found tempting is to create a small .pm, say named
AdjustDump.pm.  This would share some rules with the existing
AdjustUpgrade.pm, which would be fine IMO even if there is a small
overlap, documenting the dependency between each module.  That makes
the integration with the buildfarm much simpler by not creating more
dependencies with the modules shared between core and the buildfarm
code.  For the "shorter" part, one idea that I had is to apply to the
dump a regexp that wipes out the column definitions within the
parenthesis, keeping around the CREATE TABLE and any other attributes
not impacted by the reordering.  All that should be documented in the
module, of course.

Another thing would be to improve the backend so as we are able to
a better support for physical column ordering, which would, I assume
(and correct me if I'm wrong!), prevent the reordering of the
attributes like in this inheritance case.  But that would not address
the case of dumps taken from older versions with a new version of
pg_dump, which is something that may be interesting to have more tests
for in the long-term.  Overall a module sounds like a better solution.
--
Michael

Attachment

signature.asc

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

05 June 2024, 11:39:58

On Tue, Jun 4, 2024 at 4:28 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Apr 26, 2024 at 06:38:22PM +0530, Ashutosh Bapat wrote:
> Some points for discussion:
> 1. The test still hardcodes the diffs between two dumps. Haven't found a
> better way to do it. I did consider removing the problematic objects from
> the regression database but thought against it since we would lose some
> coverage.
>
> 2. The new code tests dump and restore of just the regression database and
> does not use pg_dumpall like pg_upgrade. Should it instead perform
> pg_dumpall? I decided against it since a. we are interested in dumping and
> restoring objects left behind by regression, b. I didn't find a way to
> provide the format option to pg_dumpall. The test could be enhanced to use
> different dump formats.
>
> I have added it to the next commitfest.
> https://commitfest.postgresql.org/48/4956/

Ashutosh and I have discussed this patch a bit last week. Here is a
short summary of my input, after I understood what is going on.

+ # We could avoid this by dumping the database loaded from original dump.
+ # But that would change the state of the objects as left behind by the
+ # regression.
+ my $expected_diff = " --
+ CREATE TABLE public.gtestxx_4 (
+- b integer,
+- a integer NOT NULL
++ a integer NOT NULL,
++ b integer
+ )
[...]
+ my ($stdout, $stderr) =
+ run_command([ 'diff', '-u', $dump4_file, $dump5_file]);
+ # Clear file names, line numbers from the diffs; those are not going to
+ # remain the same always. Also clear empty lines and normalize new line
+ # characters across platforms.
+ $stdout =~ s/^\@\@.*$//mg;
+ $stdout =~ s/^.*$dump4_file.*$//mg;
+ $stdout =~ s/^.*$dump5_file.*$//mg;
+ $stdout =~ s/^\s*\n//mg;
+ $stdout =~ s/\r\n/\n/g;
+ $expected_diff =~ s/\r\n/\n/g;
+ is($stdout, $expected_diff, 'old and new dumps match after dump and restore');
+}

I am not a fan of what this patch does, adding the knowledge related
to the dump filtering within 002_pg_upgrade.pl. Please do not take
me wrong, I am not against the idea of adding that within this
pg_upgrade test to save from one full cycle of `make check` to check
the consistency of the dump. My issue is that this logic should be
externalized, and it should be in fewer lines of code.

For the externalization part, Ashutosh and I considered a few ideas,
but one that we found tempting is to create a small .pm, say named
AdjustDump.pm. This would share some rules with the existing
AdjustUpgrade.pm, which would be fine IMO even if there is a small
overlap, documenting the dependency between each module. That makes
the integration with the buildfarm much simpler by not creating more
dependencies with the modules shared between core and the buildfarm
code. For the "shorter" part, one idea that I had is to apply to the
dump a regexp that wipes out the column definitions within the
parenthesis, keeping around the CREATE TABLE and any other attributes
not impacted by the reordering. All that should be documented in the
module, of course.

Thanks for the suggestion. I didn't understand the dependency with the buildfarm module. Will the new module be used in buildfarm separately? I will work on this soon. Thanks for changing CF entry to WoA.

Another thing would be to improve the backend so as we are able to
a better support for physical column ordering, which would, I assume
(and correct me if I'm wrong!), prevent the reordering of the
attributes like in this inheritance case. But that would not address
the case of dumps taken from older versions with a new version of
pg_dump, which is something that may be interesting to have more tests
for in the long-term. Overall a module sounds like a better solution.

Changing the physical order of column of a child table based on the inherited table seems intentional as per MergeAttributes(). That logic looks sane by itself. In binary mode pg_dump works very hard to retain the column order by issuing UPDATE commands against catalog tables. I don't think mimicking that behaviour is the right choice for non-binary dump. I agree with your conclusion that we fix it in by fixing the diffs. The code to do that will be part of a separate module.

Best Wishes,

Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Michael Paquier

Date:

05 June 2024, 23:37:42

On Wed, Jun 05, 2024 at 05:09:58PM +0530, Ashutosh Bapat wrote:
> Thanks for the suggestion. I didn't understand the dependency with the
> buildfarm module. Will the new module be used in buildfarm separately? I
> will work on this soon. Thanks for changing CF entry to WoA.

I had some doubts about PGBuild/Modules/TestUpgradeXversion.pm, but
after double-checking it loads dynamically AdjustUpgrade from the core
tree based on the base path where all the modules are:
    # load helper module from source tree
    unshift(@INC, "$srcdir/src/test/perl");
    require PostgreSQL::Test::AdjustUpgrade;
    PostgreSQL::Test::AdjustUpgrade->import;
    shift(@INC);

It would be annoying to tweak the buildfarm code more to have a
different behavior depending on the branch of Postgres tested.
Anyway, from what I can see, you could create a new module with the
dump filtering rules that AdjustUpgrade requires without having to
update the buildfarm code.

> Changing the physical order of column of a child table based on the
> inherited table seems intentional as per MergeAttributes(). That logic
> looks sane by itself. In binary mode pg_dump works very hard to retain the
> column order by issuing UPDATE commands against catalog tables. I don't
> think mimicking that behaviour is the right choice for non-binary dump. I
> agree with your conclusion that we fix it in by fixing the diffs. The code
> to do that will be part of a separate module.

Thanks.
--
Michael

Attachment

signature.asc

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

28 June 2024, 12:30:07

Sorry for delay, but here's next version of the patchset for review.

On Thu, Jun 6, 2024 at 5:07 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jun 05, 2024 at 05:09:58PM +0530, Ashutosh Bapat wrote:
> Thanks for the suggestion. I didn't understand the dependency with the
> buildfarm module. Will the new module be used in buildfarm separately? I
> will work on this soon. Thanks for changing CF entry to WoA.

I had some doubts about PGBuild/Modules/TestUpgradeXversion.pm, but
after double-checking it loads dynamically AdjustUpgrade from the core
tree based on the base path where all the modules are:
# load helper module from source tree
unshift(@INC, "$srcdir/src/test/perl");
require PostgreSQL::Test::AdjustUpgrade;
PostgreSQL::Test::AdjustUpgrade->import;
shift(@INC);

It would be annoying to tweak the buildfarm code more to have a
different behavior depending on the branch of Postgres tested.
Anyway, from what I can see, you could create a new module with the
dump filtering rules that AdjustUpgrade requires without having to
update the buildfarm code.

The two filtering rules that I picked from AdjustUpgrade() are a. use unix style newline b. eliminate blank lines. I think we could copy those rule into the new module (as done in the patch) without creating any dependency between modules. There's little gained by creating another perl function just for those two sed commands. There's no way to do that otherwise. If we keep those two modules independent, we will be free to change each module as required in future. Do we need to change buildfarm code to load the AdjustDump module like above? I am not familiar with the buildfarm code.

Here's a description of patches and some notes

0001

-------

1. Per your suggestion the logic to handle dump output differences is externalized in PostgreSQL::Test::AdjustDump. Instead of eliminating those differences altogether from both the dump outputs, the corresponding DDL in the original dump output is adjusted to look like that from the restored database. Thus we retain full knowledge of what differences to expect.

2. I have changed the name filter_dump to filter_dump_for_upgrade so as to differentiate between two adjustments 1. for upgrade and 2. for dump/restore. Ideally the name should have been adjust_dump_for_ugprade() . It's more of an adjustment than filtering as indicated by the function it calls. But I haven't changed that. The new function to adjust dumps for dump and restore tests is named adjust_dump_for_restore() however.

3. As suggested by Daniel upthread, the test for dump and restore happens before upgrade which might change the old cluster thus changing the state of objects left behind by regression. The test is not executed if regression is not used to create the old cluster.

4. The code to compare two dumps and report differences if any is moved to its own function compare_dumps() which is used for both upgrade and dump/restore tests.

The test uses the custom dump format for dumping and restoring the database.

0002

------

This commit expands the previous test to test all dump formats. But as expected that increases the time taken by this test. On my laptop 0001 takes approx 28 seconds to run the test and with 0002 it takes approx 35 seconds. But there's not much impact on the duration of running all the tests (2m30s vs 2m40s). The code which creates the DDL statements in the dump is independent of the dump format. So usually we shouldn't require to test all the formats in this test. But each format stores the dependencies between dumped objects in a different manner which would be tested with the changes in this patch. I think this patch is also useful. If we decide to keep this test, the patch is intended to be merged into 0001.

Best Wishes,

Ashutosh Bapat

Attachment

Re: Test to dump and restore objects left behind by regression

From

Michael Paquier

Date:

05 July 2024, 05:29:26

On Fri, Jun 28, 2024 at 06:00:07PM +0530, Ashutosh Bapat wrote:
> Here's a description of patches and some notes
> 0001
> -------
> 1. Per your suggestion the logic to handle dump output differences is
> externalized in PostgreSQL::Test::AdjustDump. Instead of eliminating those
> differences altogether from both the dump outputs, the corresponding DDL in
> the original dump output is adjusted to look like that from the restored
> database. Thus we retain full knowledge of what differences to expect.
> 2. I have changed the name filter_dump to filter_dump_for_upgrade so as to
> differentiate between two adjustments 1. for upgrade and 2. for
> dump/restore. Ideally the name should have been adjust_dump_for_ugprade() .
> It's more of an adjustment than filtering as indicated by the function it
> calls. But I haven't changed that. The new function to adjust dumps for
> dump and restore tests is named adjust_dump_for_restore() however.
> 3. As suggested by Daniel upthread, the test for dump and restore happens
> before upgrade which might change the old cluster thus changing the state
> of objects left behind by regression. The test is not executed if
> regression is not used to create the old cluster.
> 4. The code to compare two dumps and report differences if any is moved to
> its own function compare_dumps() which is used for both upgrade and
> dump/restore tests.
> The test uses the custom dump format for dumping and restoring the
> database.

At quick glance, that seems to be going in the right direction.  Note
that you have forgotten install and uninstall rules for the new .pm
file.

0002 increases more the runtime of a test that's already one of the
longest ones in the tree is not really appealing, I am afraid.
--
Michael

Attachment

signature.asc

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

08 July 2024, 10:29:30

On Fri, Jul 5, 2024 at 10:59 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Jun 28, 2024 at 06:00:07PM +0530, Ashutosh Bapat wrote:
> Here's a description of patches and some notes
> 0001
> -------
> 1. Per your suggestion the logic to handle dump output differences is
> externalized in PostgreSQL::Test::AdjustDump. Instead of eliminating those
> differences altogether from both the dump outputs, the corresponding DDL in
> the original dump output is adjusted to look like that from the restored
> database. Thus we retain full knowledge of what differences to expect.
> 2. I have changed the name filter_dump to filter_dump_for_upgrade so as to
> differentiate between two adjustments 1. for upgrade and 2. for
> dump/restore. Ideally the name should have been adjust_dump_for_ugprade() .
> It's more of an adjustment than filtering as indicated by the function it
> calls. But I haven't changed that. The new function to adjust dumps for
> dump and restore tests is named adjust_dump_for_restore() however.
> 3. As suggested by Daniel upthread, the test for dump and restore happens
> before upgrade which might change the old cluster thus changing the state
> of objects left behind by regression. The test is not executed if
> regression is not used to create the old cluster.
> 4. The code to compare two dumps and report differences if any is moved to
> its own function compare_dumps() which is used for both upgrade and
> dump/restore tests.
> The test uses the custom dump format for dumping and restoring the
> database.

At quick glance, that seems to be going in the right direction. Note
that you have forgotten install and uninstall rules for the new .pm
file.

Before submitting the patch, I looked for all the places which mention AdjustUpgrade or AdjustUpgrade.pm to find places where the new module needs to be mentioned. But I didn't find any. AdjustUpgrade is not mentioned in src/test/perl/Makefile or src/test/perl/meson.build. Do we want to also add AdjustUpgrade.pm in those files?

0002 increases more the runtime of a test that's already one of the
longest ones in the tree is not really appealing, I am afraid.

We could forget 0002. I am fine with that. But I can change the code such that formats other than "plain" are tested when PG_TEST_EXTRAS contains "regress_dump_formats". Would that be acceptable?

Best Wishes,

Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Michael Paquier

Date:

09 July 2024, 07:36:59

On Mon, Jul 08, 2024 at 03:59:30PM +0530, Ashutosh Bapat wrote:
> Before submitting the patch, I looked for all the places which mention
> AdjustUpgrade or AdjustUpgrade.pm to find places where the new module needs
> to be mentioned. But I didn't find any. AdjustUpgrade is not mentioned
> in src/test/perl/Makefile or src/test/perl/meson.build. Do we want to also
> add AdjustUpgrade.pm in those files?

Good question.  This has not been mentioned on the thread that added
the module:
https://www.postgresql.org/message-id/891521.1673657296%40sss.pgh.pa.us

And I could see it as being useful if installed.  The same applies to
Kerberos.pm, actually.  I'll ping that on a new thread.

> We could forget 0002. I am fine with that.  But I can change the code such
> that formats other than "plain" are tested when PG_TEST_EXTRAS contains
> "regress_dump_formats". Would that be acceptable?

Interesting idea.  That may be acceptable, under the same arguments as
the xid_wraparound one.
--
Michael

Attachment

signature.asc

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

12 July 2024, 05:12:35

On Tue, Jul 9, 2024 at 1:07 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jul 08, 2024 at 03:59:30PM +0530, Ashutosh Bapat wrote:
> > Before submitting the patch, I looked for all the places which mention
> > AdjustUpgrade or AdjustUpgrade.pm to find places where the new module needs
> > to be mentioned. But I didn't find any. AdjustUpgrade is not mentioned
> > in src/test/perl/Makefile or src/test/perl/meson.build. Do we want to also
> > add AdjustUpgrade.pm in those files?
>
> Good question.  This has not been mentioned on the thread that added
> the module:
> https://www.postgresql.org/message-id/891521.1673657296%40sss.pgh.pa.us
>
> And I could see it as being useful if installed.  The same applies to
> Kerberos.pm, actually.  I'll ping that on a new thread.

For now, it may be better to maintain status-quo. If we see a need to
use these modules in future by say extensions or tests outside core
tree, we will add them to meson and make files.

>
> > We could forget 0002. I am fine with that.  But I can change the code such
> > that formats other than "plain" are tested when PG_TEST_EXTRAS contains
> > "regress_dump_formats". Would that be acceptable?
>
> Interesting idea.  That may be acceptable, under the same arguments as
> the xid_wraparound one.

Done. Added a new entry in PG_TEST_EXTRA documentation.

I have merged the two patches now.


--
Best Wishes,
Ashutosh Bapat

Attachment

0001-Test-pg_dump-restore-of-regression-objects-20240712.patch

Re: Test to dump and restore objects left behind by regression

From

Tom Lane

Date:

31 October 2024, 17:26:01

Michael Paquier <michael@paquier.xyz> writes:
> On my laptop, testing the plain format adds roughly 12s, in a test
> that now takes 1m20s to run vs 1m32s.  Enabling regress_dump_formats
> and adding three more formats counts for 45s of runtime.  For a test
> that usually shows up as the last one to finish for a heavily
> parallelized run.  So even the default of "plain" is going to be
> noticeable, I am afraid.

Yeah, that's what I've been afraid of from the start.  There's
no way that this will buy us enough new coverage to justify
that sort of addition to every check-world run.

I'd be okay with adding it in a form where the default behavior
is to do no additional checking.  Whether that's worth maintaining
is hard to say though.

            regards, tom lane

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

07 November 2024, 13:29:03

Hi Tom and Michael,

Thanks for your inputs.

I am replying to all the comments in a single email arranging related
comments together.

On Thu, Oct 31, 2024 at 11:26 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On my laptop, testing the plain format adds roughly 12s, in a test
> that now takes 1m20s to run vs 1m32s.  Enabling regress_dump_formats
> and adding three more formats counts for 45s of runtime.  For a test
> that usually shows up as the last one to finish for a heavily
> parallelized run.  So even the default of "plain" is going to be
> noticeable, I am afraid.

> On Thu, Oct 31, 2024 at 10:26:01AM -0400, Tom Lane wrote:
> > I'd be okay with adding it in a form where the default behavior
> > is to do no additional checking.

If I run the test alone, it takes 45s (master) vs 54s (with patch) on
my laptop. These readings are similar to what you have observed. The
restore step by itself takes most of the time, even if a. we eliminate
data, b. use formats other than plain or c. use --jobs=2. Hence I am
fine with Tom's suggestion i.e. default behaviour is to do no
additional testing. I propose to test all dump formats (including
plain) only when PG_TEST_EXTRA has "regress_dump_tests". But see next

>
> What's the advantage of testing all the formats?  Would that stuff
> have been able to catch up more issues related to specific format(s)
> when it came to the compression improvements with inheritance?

I haven't caught any more issues with formats other than "plain". It
is more for future-proof testing. I am fine if we want to test just
plain dump format for now. Adding more formats would be easier if
required.

>> Whether that's worth maintaining
> > is hard to say though.
>
> In terms of maintenance, it would be nice if we are able to minimize
> the code added to the pg_upgrade suite, so as it would be simple to
> switch this code elsewhere if need be.

I think Tom hints at maintenance of
AdjustDump::adjust_dump_for_restore(). In future, if the difference
between dump from the original database and that from the restored
database grows, we will need to update
AdjustDump::adjust_dump_for_restore() accordingly. That will be some
maintenance. But the person introducing such changes will get a chance
to fix them if unintentional. That balances out any maintenance
efforts, I think.

>
> +   test_regression_dump_restore($oldnode, %node_params);
>
> Why is this only done for the main regression test suite?  Perhaps it
> could be useful as well for tests that want to check after their own
> custom dumps, as a shortcut?
>
> Linked to that.  Could there be some use in being able to pass down a
> list of databases to this routine, rather than being limited only to
> "regression"?  Think extension databases with USE_MODULE_DB that have
> unique names.

I did think of it when implementing this function. In order to test
the custom dumps or extensions, adjust_regress_dumpfile() will need to
be extensible or the test will need a way to accept a custom dump file
for comparison. Without a concrete use case, adding the customization
hooks might go wrong and will need rework.
test_regression_dump_restore() itself is isolated enough that we can
extend it when the need arises. When the need arises we will know what
needs to be extensible and how. If you have a specific use case,
please let me know, I will accommodate it in my patch.

> Perhaps we
> should be more ambitious and move more logic into AdjustDump.pm?  If
> we think that the full cycle of dump -> restore -> dump -> compare
> could be used elsewhere, this would limit the footprint of what we are
> doing in the pg_upgrade script in this patch and be able to do similar
> stuff in out-of-core extensions or other tests.  Let's say a
> PostgreSQL::Test::Dump.pm?

dump->restore->dump->compare pattern is seen only in 002_pg_upgrade
test. 002_compare_backups compares dumps from servers but does not use
the dump->restore->dump->compare pattern. If a similar pattern starts
appearing at multiple places, we will easily move
test_regression_dump_restore() to a common module to avoid code
duplication. That function is isolated enough for that purpose.

> - Dump of a database into an output file given in input, as a routine
> of Cluster.pm so as it is possible to do dumps from different major
> versions.  Format should be defined in input.

SInce you are suggesting adding the new routine to Cluster.pm, I
assume that you would like to use it in many tests (ideally every test
which uses pg_dump). I did attempt this when I wrote the last version
of the patch. Code to run a pg_dump command is just a few lines. The
tests invoke pg_dump in many different ways with many different
combinations of arguments. In order to cater all those invocations,
the routine in Cluster.pm needs to be very versatile and thus complex.
It will be certainly a dozen lines at least. If such a routine would
have been useful, it would have been added to Cluster.pm already. It's
not there, because it won't be useful.

We could turn the two invocations of pg_dump for comparison (in the
patch) into a routine if that helps. It might shave a few lines of
code. Since the routine won't be general, it should reside in
002_pg_upgrade where it is used.

If you have something else in mind, please let me know.

> - Restore to a database from an input file, also as a routine of
> Cluster.pm, for the major version argument.

Similar to above, each of the pg_restore invocations are just a few
lines but there is a lot of variety in those invocations.

> - Filter of the dumps for the contents where column ordering is
> inconsistent up at restore.  In a new module.

Please note, this is filtering + adjustment. The routine is already in
a new module as you suggested earlier.

>
> I'm wondering if it would make sense to also externalize the dump
> comparison routine currently in the pg_upgrade script.
> - Comparison of two dumps, with potentially filters applied to them,
> with diff printed.  In a new module.

It is a good idea to externalize the compare_dump() function in
PostgreSQL::Test::Utils. Similar code exists in
002_compare_backups.pl. 027_stream_regress.pl also uses compare() to
compare dump files but it uses `diff` command for the same. We can
change both usages to use compare_dump().

>
> +   # Dump the original database in "plain" format for comparison later. The
> +   # order of columns in COPY statements dumped from the original database and
> [...]
> +   # Adjust the CREATE TABLE ... INHERITS statements.
> +   if ($original)
> +   {
> +       $dump =~ s/(^CREATE\sTABLE\sgenerated_stored_tests\.gtestxx_4\s\()
> +                  (\n\s+b\sinteger),
> +                  (\n\s+a\sinteger)/$1$3,$2/mgx;
>
> The reason why $original exists is documented partially in both
> 002_pg_upgrade.pl and AdjustDump.pm.  It would be better to
> consolidate that only in AdjustDump.pm, I guess.

I believe the comment in 0002_pg_upgrade.pl you quoted above and the
prologue of adjust_regress_dumpfile() are the two places you are
referring to. They serve different purposes. The one in 002_pg_upgrade
explains why we dump only schema for comparison. It is independent of
whether the dump is taken from the original database or target
database. The argument "original" to adjust_regress_dumpfile() is only
explained in the function's prologue in AdjustDump.pm. Am I missing
something?

> Isn't the name
> "$original" a bit too general when it comes to applying filters to
> the dumps to as the order of the column re-dumped is under control?
> Perhaps it would be adapted to use a hash that can be extended with
> more than one parameter to control which filters are applied?  For
> example, imagine a %params where the caller of adjust_dumpfile() can
> pass in a "filter_column_order => 1".  The filters applied to the dump
> are then self-documented.  We could do with a second for the
> whitespaces, as well.

I agree that "original" is a generic name. And I like your suggestion
partly. I will rename it as "adjust_column_order".

But I don't think we need to use a hash since filters like whitespace
are not dependent upon whether the dump is from source or target
database. IOW those filters are not optional. It will add extra
redirection unnecessarily. If in future we have to add another
adjustment which is applicable under certain conditions, we could use
a hash of switches but till then let's keep it simple.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Daniel Gustafsson

Date:

18 December 2024, 17:09:31

> On 18 Dec 2024, at 12:28, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:

In general I think it's fine to have such an expensive test gated behind a
PG_TEST_EXTRA flag, and since it's only run on demand we might as well run it
for all formats while at it.  If this ran just once per week in the buildfarm
it would still allow us to catch things in time at fairly low overall cost.

> I have rebased my patches on the current HEAD. The test now passes and
> does not show any new diff or bug.

A few comments on this version of the patch:

+   regression run. Not enabled by default because it is time consuming.
Since this test consumes both time and to some degree diskspace (the dumpfiles)
I wonder if this should be "time and resource consuming".


+   if (   $ENV{PG_TEST_EXTRA}
+       && $ENV{PG_TEST_EXTRA} =~ /\bregress_dump_test\b/)
Should this also test that $oldnode and $newnode have matching pg_version to
keep this from running in a cross-version upgrade test?  While it can be argued
that running this in a cross-version upgrade is breaking it and getting to keep
both pieces, it's also not ideal to run a resource intensive test we know will
fail.  (It can't be done at this exact callsite, just picked to illustrate.)


-sub filter_dump
+sub filter_dump_for_upgrade
What is the reason for the rename?  filter_dump() is perhaps generic but it's
also local to the upgrade test so it's also not too unclear.


+  my $format_spec = substr($format, 0, 1);
This doesn't seem great for readability, how about storing the formats and
specfiers in an array of Perl hashes which can be iterated over with
descriptive names, like $format{'name'} and $format{'spec'}?


+     || die "opening $dump_adjusted ";
Please include the errno in the message using ": $!" appended to the error
message, it could help in debugging.

+compare the results of dump and retore tests
s/retore/restore/


+   else
+   {
+       note('first dump file: ' . $dump1);
+       note('second dump file: ' . $dump2);
+   }
+
This doesn't seem particularly helpful, if the tests don't fail then printing
the names won't bring any needed information.  What we could do here is to add
an is() test in compare_dump()s to ensure the filenames differ to catch any
programmer error in passing in the same file twice.

--
Daniel Gustafsson

Re: Test to dump and restore objects left behind by regression

From

Daniel Gustafsson

Date:

27 December 2024, 15:46:49

> On 20 Dec 2024, at 11:01, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> On Wed, Dec 18, 2024 at 7:39 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> On 18 Dec 2024, at 12:28, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:

>> +   if (   $ENV{PG_TEST_EXTRA}
>> +       && $ENV{PG_TEST_EXTRA} =~ /\bregress_dump_test\b/)
>> Should this also test that $oldnode and $newnode have matching pg_version to
>> keep this from running in a cross-version upgrade test?  While it can be argued
>> that running this in a cross-version upgrade is breaking it and getting to keep
>> both pieces, it's also not ideal to run a resource intensive test we know will
>> fail.  (It can't be done at this exact callsite, just picked to illustrate.)
>
> You already wrote it in parenthesis. At the exact callsite $oldnode
> and $newnode can not be of different versions. In fact newnode is yet
> to be created at this point. But $oldnode has the same version as the
> server run from the code. In a cross-version upgrade this test will
> not be executed. I am confused as to what this comment is about.

Sure, it can't be checked until $newnode is created, but it seems like a cheap
test to ensure it's not executed as part of someones cross-version tests.

>> +  my $format_spec = substr($format, 0, 1);
>> This doesn't seem great for readability, how about storing the formats and
>> specfiers in an array of Perl hashes which can be iterated over with
>> descriptive names, like $format{'name'} and $format{'spec'}?
>
> Instead of an array of hashes, I used a single hash with format
> description as key and format spec as value. Hope that's acceptable.

LGTM.

--
Daniel Gustafsson

Re: Test to dump and restore objects left behind by regression

From

Alvaro Herrera

Date:

06 February, 12:43:56

On 2025-Feb-06, Michael Paquier wrote:

> On Wed, Feb 05, 2025 at 03:28:04PM +0900, Michael Paquier wrote:
> > Hmm.  I was reading through the patch and there is something that
> > clearly stands out IMO: the new compare_dumps().  It is in Utils.pm,
> > and it acts as a wrapper of `diff` with its formalized output format.
> > It is not really about dumps, but about file comparisons.  This should
> > be renamed compare_files(), with internals adjusted as such, and
> > reused in all the existing tests.  Good idea to use that in
> > 027_stream_regress.pl, actually.  I'll go extract that first, to
> > reduce the presence of `diff` in the whole set of TAP tests.
> 
> The result of this part is pretty neat, resulting in 0001 where it is
> possible to use the refactored routine as well in pg_combinebackup
> where there is a piece comparing dumps.  There are three more tests
> with diff commands and assumptions of their own, that I've left out.

Great, I've looked at doing something like this in the libpq_pipeline
test for better diff reporting -- what I have uses Test::Differences,
which is pretty neat and usable, but it's not part of the standard
installed perl modules, which is a large downside.  I can probably get
rid of my hack once you get 0001 in.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Find a bug in a program, and fix it, and the program will work today.
Show the program how to find and fix a bug, and the program
will work forever" (Oliver Silfridge)

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

11 February, 09:49:33

On Thu, Feb 6, 2025 at 11:32 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Feb 05, 2025 at 03:28:04PM +0900, Michael Paquier wrote:
> > Hmm.  I was reading through the patch and there is something that
> > clearly stands out IMO: the new compare_dumps().  It is in Utils.pm,
> > and it acts as a wrapper of `diff` with its formalized output format.
> > It is not really about dumps, but about file comparisons.  This should
> > be renamed compare_files(), with internals adjusted as such, and
> > reused in all the existing tests.  Good idea to use that in
> > 027_stream_regress.pl, actually.  I'll go extract that first, to
> > reduce the presence of `diff` in the whole set of TAP tests.
>
> The result of this part is pretty neat, resulting in 0001 where it is
> possible to use the refactored routine as well in pg_combinebackup
> where there is a piece comparing dumps.  There are three more tests
> with diff commands and assumptions of their own, that I've left out.
> This has the merit of unifying the output generated should any diffs
> show up, while removing a nice chunk from the main patch.

Sorry for replying late here. The refactored code in
002_compare_backups.pl has a potential to cause confusion even without
this refactoring. The differences in tablespace paths are adjusted in
compare_files() and not in the actual dump outputs. In case there's a
difference other than paths, diff between the dump outputs is reported
which will also show the differences in paths. That might mislead
developers in thinking that the differences in paths are also not
expected. Am I right?

I will address other comments soon, but the answer to this question
has some impact there.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

12 February, 07:35:47

On Wed, Feb 12, 2025 at 5:25 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Feb 11, 2025 at 12:19:33PM +0530, Ashutosh Bapat wrote:
> > Sorry for replying late here. The refactored code in
> > 002_compare_backups.pl has a potential to cause confusion even without
> > this refactoring. The differences in tablespace paths are adjusted in
> > compare_files() and not in the actual dump outputs. In case there's a
> > difference other than paths, diff between the dump outputs is reported
> > which will also show the differences in paths. That might mislead
> > developers in thinking that the differences in paths are also not
> > expected. Am I right?
>
> Logically, 002_compare_backups.pl is still the same, isn't it?  We're
> still passing the file paths to compare_text(), except that the
> comparison routine is given as an argument one level higher.

Yes. That's right. Not something introduced by
169208092f5c98a6021b23b38f03a5d65f84ad96.

>
> You are right that there could be an argument for changing the files
> are they are on-disk, and do a diff based on what's on disk after what
> has changed so as the filtered parts are out of the report. However,
> there is also an argument for not changing them as that's more useful
> to know the original state of the dump for debugging.  This one
> involves only a small change, which is OK as-is, IMHO.

Fine. We know what to fix if an ambiguity arises in future.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

25 February, 09:29:24

On Tue, Feb 11, 2025 at 5:53 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Michael,
>
>
> On Sun, Feb 9, 2025 at 1:25 PM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Fri, Feb 07, 2025 at 07:11:25AM +0900, Michael Paquier wrote:
> > > Okay, thanks for the feedback.  We have been relying on diff -u for
> > > the parts of the tests touched by 0001 for some time now, so if there
> > > are no objections I would like to apply 0001 in a couple of days.
> >
> > This part has been applied as 169208092f5c.
>
> Thanks. PFA rebased patches.

PFA rebased patches.

After rebasing I found another bug and reported it at [1].

For the time being I have added --no-statistics to the pg_dump command
when taking a dump for comparison.

[1] https://www.postgresql.org/message-id/CAExHW5vf9D+8-a5_BEX3y=2y_xY9hiCxV1=C+FnxDvfprWvkng@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat

Attachment

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

11 March, 13:44:45

On Tue, Feb 25, 2025 at 11:59 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 5:53 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > Hi Michael,
> >
> >
> > On Sun, Feb 9, 2025 at 1:25 PM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > On Fri, Feb 07, 2025 at 07:11:25AM +0900, Michael Paquier wrote:
> > > > Okay, thanks for the feedback.  We have been relying on diff -u for
> > > > the parts of the tests touched by 0001 for some time now, so if there
> > > > are no objections I would like to apply 0001 in a couple of days.
> > >
> > > This part has been applied as 169208092f5c.
> >
> > Thanks. PFA rebased patches.
>
> PFA rebased patches.
>
> After rebasing I found another bug and reported it at [1].

This bug has been fixed. But now that it's fixed, it's easy to see
another bug related to materialized view statistics. I have reported
it at [2]. That's the fourth bug identified by this test.

>
> For the time being I have added --no-statistics to the pg_dump command
> when taking a dump for comparison.
>

I have not taken out this option because of materialized view bug.

> [1] https://www.postgresql.org/message-id/CAExHW5vf9D+8-a5_BEX3y=2y_xY9hiCxV1=C+FnxDvfprWvkng@mail.gmail.com

[2] https://www.postgresql.org/message-id/CAExHW5s47kmubpbbRJzSM-Zfe0Tj2O3GBagB7YAyE8rQ-V24Uw@mail.gmail.com


--
Best Wishes,
Ashutosh Bapat

Here are patches missing in the previous email.

On Thu, Mar 13, 2025 at 6:09 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Mar 13, 2025 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > Hello
> >
> > On 2025-Mar-13, Ashutosh Bapat wrote:
> >
> > > 1. can you please run the test again and share the dump outputs. They
> > > will be located in a temporary directory with names
> > > src_dump.sql_adjusted and dest_dump.<format>.sql_adjusted.
> >
> > Ah, I see the problem :-)  The first initdb does this:
> >
> >         # Running: initdb -D
/home/alvherre/Code/pgsql-build/master/src/bin/pg_upgrade/tmp_check/t_002_pg_upgrade_old_node_data/pgdata-A trust -N
--wal-segsize1 --allow-group-access --encoding UTF-8 --lc-collate C --lc-ctype C --locale-provider builtin
--builtin-localeC.UTF-8 -k 
> >         The files belonging to this database system will be owned by user "alvherre".
> >         This user must also own the server process.
> >
> >         The database cluster will be initialized with this locale configuration:
> >           locale provider:   builtin
> >           default collation: C.UTF-8
> >           LC_COLLATE:  C
> >           LC_CTYPE:    C
> >           LC_MESSAGES: C
> >           LC_MONETARY: es_CL.UTF-8
> >           LC_NUMERIC:  es_CL.UTF-8
> >           LC_TIME:     es_CL.UTF-8
> >         The default text search configuration will be set to "english".
> >
> >         Data page checksums are enabled.
> >
> > which for some reason used my environment setting for LC_MONETARY.
> >
>
> Thanks. This is super helpful. I am able to reproduce the problem
> $ unset LC_MONETARY
> $ export PG_TEST_EXTRA=regress_dump_test
> $ meson test --suite setup && meson test pg_upgrade/002_pg_upgrade
> ... snip ...
> 1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade        OK
>        72.38s   44 subtests passed
>
>
> Ok:                 1
> Expected Fail:      0
> Fail:               0
> Unexpected Pass:    0
> Skipped:            0
> Timeout:            0
>
> Full log written to
> /home/ashutosh/work/units/pg_dump_test/build/dev/meson-logs/testlog.txt
> $ export LC_MONETARY="es_CL.UTF-8"
> $ meson test --suite setup && meson test pg_upgrade/002_pg_upgrade
> ... snip ...
> 1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade        ERROR
>        69.18s   exit status 4
> >>> with_icu=no
LD_LIBRARY_PATH=/home/ashutosh/work/units/pg_dump_test/build/dev/tmp_install//home/ashutosh/work/units/pg_dump_test/build/dev/lib/x86_64-linux-gnu
REGRESS_SHLIB=/home/ashutosh/work/units/pg_dump_test/build/dev/src/test/regress/regress.so
PATH=/home/ashutosh/work/units/pg_dump_test/build/dev/tmp_install//home/ashutosh/work/units/pg_dump_test/build/dev/bin:/home/ashutosh/work/units/pg_dump_test/build/dev/src/bin/pg_upgrade:/home/ashutosh/work/units/pg_dump_test/build/dev/src/bin/pg_upgrade/test:/home/ashutosh/work/units/pg_dump_test/build/dev/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/snap/bin
MALLOC_PERTURB_=30
share_contrib_dir=/home/ashutosh/work/units/pg_dump_test/build/dev/tmp_install//home/ashutosh/work/units/pg_dump_test/build/dev/share/postgresql/contrib
PG_REGRESS=/home/ashutosh/work/units/pg_dump_test/build/dev/src/test/regress/pg_regress
top_builddir=/home/ashutosh/work/units/pg_dump_test/build/dev
INITDB_TEMPLATE=/home/ashutosh/work/units/pg_dump_test/build/dev/tmp_install/initdb-template/usr/bin/python3
/home/ashutosh/work/units/pg_dump_test/build/dev/../../coderoot/pg/src/tools/testwrap--basedir
/home/ashutosh/work/units/pg_dump_test/build/dev--srcdir
/home/ashutosh/work/units/pg_dump_test/coderoot/pg/src/bin/pg_upgrade--pg-test-extra '' --testgroup pg_upgrade
--testname002_pg_upgrade -- /usr/bin/perl -I /home/ashutosh/work/units/pg_dump_test/coderoot/pg/src/test/perl -I
/home/ashutosh/work/units/pg_dump_test/coderoot/pg/src/bin/pg_upgrade
/home/ashutosh/work/units/pg_dump_test/coderoot/pg/src/bin/pg_upgrade/t/002_pg_upgrade.pl
>
>
>
> Ok:                 0
> Expected Fail:      0
> Fail:               1
> Unexpected Pass:    0
> Skipped:            0
> Timeout:            0
>
> I see what's happening.  If I set LC_MONETARY environment explicitly,
> that's taken by initdb
> $ export LC_MONETARY="es_CL.UTF-8";rm -rf $DataDir; $BinDir/initdb -D
> $DataDir -A trust -N --wal-segsize 1 --allow-group-access --encoding
> UTF-8 --lc-collate C --lc-ctype C --locale-provider builtin
> --builtin-locale C.UTF-8 -k
> The files belonging to this database system will be owned by user "ashutosh".
> This user must also own the server process.
>
> The database cluster will be initialized with this locale configuration:
>   locale provider:   builtin
>   default collation: C.UTF-8
>   LC_COLLATE:  C
>   LC_CTYPE:    C
>   LC_MESSAGES: en_US.UTF-8
>   LC_MONETARY: es_CL.UTF-8
>   LC_NUMERIC:  en_US.UTF-8
>   LC_TIME:     en_US.UTF-8
> The default text search configuration will be set to "english".
>
> If I don't set it explicitly, it's taken from default settings
> $ unset LC_MONETARY;rm -rf $DataDir; $BinDir/initdb -D $DataDir -A
> trust -N --wal-segsize 1 --allow-group-access --encoding UTF-8
> --lc-collate C --lc-ctype C --locale-provider builtin --builtin-locale
> C.UTF-8 -k
> The files belonging to this database system will be owned by user "ashutosh".
> This user must also own the server process.
>
> The database cluster will be initialized with this locale configuration:
>   locale provider:   builtin
>   default collation: C.UTF-8
>   LC_COLLATE:  C
>   LC_CTYPE:    C
>   LC_MESSAGES: en_US.UTF-8
>   LC_MONETARY: en_US.UTF-8
>   LC_NUMERIC:  en_US.UTF-8
>   LC_TIME:     en_US.UTF-8
> The default text search configuration will be set to "english".
>
> In your case probably your default setting is es_CL.UTF-8 or have set
> LC_MONETARY explicitly in your environment.
>
> I think the fix is to explicitly pass --lc-monetary to the old cluster
> and the restored cluster. 003 patch in the attached patch set does
> that. Please check if it fixes the issue for you.
>
> Additionally we should check that it gets copied to the new cluster as
> well. But I haven't figured out how to get those settings yet. This
> treatment is similar to how --lc-collate and --lc-ctype are treated. I
> am wondering whether we should explicitly pass --lc-messages,
> --lc-time and --lc-numeric as well.
>
> 2d819a08a1cbc11364e36f816b02e33e8dcc030b introduced buildin locale
> provider and added overrides to LC_COLLATE and LC_TYPE. But it did not
> override other LC_, which I think it should have. In pure upgrade
> test, the upgraded node inherits the locale settings of the original
> cluster, so this wasn't apparent. But with pg_dump testing, the
> original and restored databases are independent. Hence I think we have
> to override all LC_* settings by explicitly mentioning --lc-* options
> to initdb. Please let me know what you think about this?
>
> --
> Best Wishes,
> Ashutosh Bapat



--
Best Wishes,
Ashutosh Bapat

Attachment

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

19 March, 14:43:12

On Thu, Mar 13, 2025 at 6:10 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> >
> > I think the fix is to explicitly pass --lc-monetary to the old cluster
> > and the restored cluster. 003 patch in the attached patch set does
> > that. Please check if it fixes the issue for you.
> >
> > Additionally we should check that it gets copied to the new cluster as
> > well. But I haven't figured out how to get those settings yet. This
> > treatment is similar to how --lc-collate and --lc-ctype are treated. I
> > am wondering whether we should explicitly pass --lc-messages,
> > --lc-time and --lc-numeric as well.
> >
> > 2d819a08a1cbc11364e36f816b02e33e8dcc030b introduced buildin locale
> > provider and added overrides to LC_COLLATE and LC_TYPE. But it did not
> > override other LC_, which I think it should have. In pure upgrade
> > test, the upgraded node inherits the locale settings of the original
> > cluster, so this wasn't apparent. But with pg_dump testing, the
> > original and restored databases are independent. Hence I think we have
> > to override all LC_* settings by explicitly mentioning --lc-* options
> > to initdb. Please let me know what you think about this?
> >

Investigated this further. The problem is that the pg_regress run
creates regression database with specific properties but the restored
database does not have those properties. That led me to a better
solution. Additionally it's local to the new test. Use --create when
dumping and restoring the regression database. This way the database
properties or "configuration variable settings (as pg_dump
documentation calls them) are copied to the restored database as well.
Those properties include LC_MONETARY. Additionally now the test covers
--create option as well.

PFA patches.

--
Best Wishes,
Ashutosh Bapat

On Fri, Mar 21, 2025 at 11:38 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-21, Ashutosh Bapat wrote:
>
> > I used the same parallelism in pg_restore and pg_dump too. And your
> > numbers seem to be similar to mine; slightly less than 20% slowdown.
> > But is that slowdown acceptable? From the earlier discussions, it
> > seems the answer is No. Haven't heard otherwise.
>
> I don't think we need to see slowdown this in relative terms, the way we
> would discuss a change in the executor.  This is not a change that
> would affect user-level stuff in any way.  We need to see it in absolute
> terms: in machines similar to mine, the pg_upgrade test would go from
> taking 23s to taking 27s.  This is 4s slower, but this isn't an increase
> in total test runtime, because decently run test suites run multiple
> tests in parallel.  This is the same that Peter said in [1].  The total
> test runtime change might not be *that* large.  I'll take a few numbers
> and report back.

 Using -j2 in pg_dump and -j3 in pg_restore does not improve timing
much on my laptop. I have used -j2 for both pg_dump and restore
instead of -j3 so as to avoid using more cores when tests are run in
parallel.

Further to reduce run time, I tried -1/--single-transaction but that's
not allowed with --create. I also tried --transaction-size=1000 but
that doesn't affect the run time of the test. Next I thought of using
standard output and input instead of files but it doesn't help since
1. directory format cannot use those and it's the only format allowing
parallelism, 2. that's slower than using files with --no-sync. Didn't
find any other way which can help us reduce the test time.

Please note that the dumps taken for comparison cannot use -j since
they are required to be in "plain" format so that text manipulation
comparison works on them.

One concern I have with directory format is the dumped database is not
readable. This might make investigating a but identified the test a
bit more complex. But I guess, in such a case investigator can either
use the dumps taken for comparison or change the code to use plain
format for investigation. So it's a price we pay for making test
faster.

Here's next patchset:
0001 - it's the same 0001 patch as previous one, includes the test
with all formats and also the PG_TEST_EXTRA option

0002 - removes PG_TEST_EXTRA and also tests only one format
--directory with -j2 with default compression. It should be merged
into 0001 before committing. This is a separate patch for now in case
we decide to go back to 0001.

0003 - same as 0002 in the previous patch set. It excludes statistics
from comparison, otherwise the test will fail because of bug reported
at [1]. Ideally we shouldn't commit this patch so as to test
statistics dump and restore, but in case we need the test to pass till
the bug is fixed, we should merge this patch to 0001 before
committing.

[1] https://www.postgresql.org/message-id/CAExHW5s47kmubpbbRJzSM-Zfe0Tj2O3GBagB7YAyE8rQ-V24Uw@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat

On Mon, Mar 24, 2025 at 5:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-24, Ashutosh Bapat wrote:
>
> > One concern I have with directory format is the dumped database is not
> > readable. This might make investigating a but identified the test a
> > bit more complex.
>
> Oh, it's readable all right.  You just need to use `pg_restore -f-` to
> read it.  No big deal.
>
>
> So I ran this a few times:
> /usr/bin/time make -j8 -Otarget -C /pgsql/build/master check-world -s PROVE_FLAGS="-c -j6" > /dev/null
>
> commenting out the call to test_regression_dump_restore() to test how
> much additional runtime does the new test incur.
>
> With test:
>
> 136.95user 116.56system 1:13.23elapsed 346%CPU (0avgtext+0avgdata 250704maxresident)k
> 4928inputs+55333008outputs (114major+14784937minor)pagefaults 0swaps
>
> 138.11user 117.43system 1:15.54elapsed 338%CPU (0avgtext+0avgdata 278592maxresident)k
> 48inputs+55333464outputs (80major+14794494minor)pagefaults 0swaps
>
> 137.05user 113.13system 1:08.19elapsed 366%CPU (0avgtext+0avgdata 279272maxresident)k
> 48inputs+55330064outputs (83major+14758028minor)pagefaults 0swaps
>
> without the new test:
>
> 135.46user 114.55system 1:14.69elapsed 334%CPU (0avgtext+0avgdata 145372maxresident)k
> 32inputs+55155256outputs (105major+14737549minor)pagefaults 0swaps
>
> 135.48user 114.57system 1:09.60elapsed 359%CPU (0avgtext+0avgdata 148224maxresident)k
> 16inputs+55155432outputs (95major+14749502minor)pagefaults 0swaps
>
> 133.76user 113.26system 1:14.92elapsed 329%CPU (0avgtext+0avgdata 148064maxresident)k
> 48inputs+55154952outputs (84major+14749531minor)pagefaults 0swaps
>
> 134.06user 113.83system 1:16.09elapsed 325%CPU (0avgtext+0avgdata 145940maxresident)k
> 32inputs+55155032outputs (83major+14738602minor)pagefaults 0swaps
>
> The increase in duration here is less than a second.
>
>
> My conclusion with these numbers is that it's not worth hiding this test
> in PG_TEST_EXTRA.

Thanks for the conclusion.

On Mon, Mar 24, 2025 at 3:29 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 24 Mar 2025, at 10:54, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>
> > 0003 - same as 0002 in the previous patch set. It excludes statistics
> > from comparison, otherwise the test will fail because of bug reported
> > at [1]. Ideally we shouldn't commit this patch so as to test
> > statistics dump and restore, but in case we need the test to pass till
> > the bug is fixed, we should merge this patch to 0001 before
> > committing.
>
> If the reported bug isn't fixed before feature freeze I think we should commit
> this regardless as it has clearly shown value by finding bugs (though perhaps
> under PG_TEST_EXTRA or in some disconnected till the bug is fixed to limit the
> blast-radius in the buildfarm).

Combining Alvaro's and Daniel's recommendations, I think we should
squash all the three of my patches while committing the test if the
bug is not fixed by then. Otherwise we should squash first two patches
and commit it. Just attaching the patches again for reference.

> If we really wanted to save some total test runtime,
> it might be better to write a regress schedule file for
> 027_stream_regress.pl which only takes the test that emit useful WAL,
> rather than all tests.

That's out of scope for this patch, but it seems like an idea worth exploring.

--
Best Wishes,
Ashutosh Bapat

On Thu, Mar 27, 2025 at 06:15:06PM +0100, Alvaro Herrera wrote:
> BTW another idea to shorten this tests's runtime might be to try and
> identify which of parallel_schedule tests leave objects behind and
> create a shorter schedule with only those (a possible implementation
> might keep a list of the slow tests that don't leave any useful object
> behind, then filter parallel_schedule to exclude those; this ensures
> test files created in the future are still used.)

I'm not much a fan of approaches that require an extra schedule,
because this is prone to forget the addition of objects that we'd want
to cover for the scope of this thread with the dump/restore
inter-dependencies, failing our goal of having more coverage.  And
history has proven that we are quite bad at maintaining multiple
schedules for the regression test suite (remember the serial one or
the standby one in pg_regress?).  So we should really do things so as
the schedules are down to a strict minimum: 1.

If we're worried about the time taken by the test (spoiler: I am and
the upgrade tests already show always as last to finish in parallel
runs), I would recommend to put that under a PG_TEST_EXTRA.  I'm OK to
add the switch to my buildfarm animals if this option is the consensus
and if it gets into the tree.
--
Michael

Attachment

signature.asc

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

28 March, 09:32:58

On Thu, Mar 27, 2025 at 10:45 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-27, Ashutosh Bapat wrote:
>
> > On Thu, Mar 27, 2025 at 6:01 PM vignesh C <vignesh21@gmail.com> wrote:
>
> > > Couple of minor thoughts:
> > > 1) I felt this error message is not conveying the error message correctly:
> > > +       if ($src_node->pg_version != $dst_node->pg_version
> > > +               or defined $src_node->{_install_path})
> > > +       {
> > > +               fail("same version dump and restore test using default
> > > installation");
> > > +               return;
> > > +       }
> > >
> > > how about something like below:
> > > fail("source and destination nodes must have the same PostgreSQL
> > > version and default installation paths");
> >
> > The text in ok(), fail() etc. are test names and not error messages.
> > See [1]. Your suggestion and other versions that I came up with became
> > too verbose to be test names. So I think the text here is compromise
> > between conveying enough information and not being too long. We
> > usually have to pick the testname and lookup the test code to
> > investigate the failure. This text serves that purpose.
>
> Maybe
> fail("roundtrip dump/restore of the regression database")

No, that's losing some information like default installation and the
same version.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

28 March, 09:50:29

On Fri, Mar 28, 2025 at 7:07 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Mar 27, 2025 at 06:15:06PM +0100, Alvaro Herrera wrote:
> > BTW another idea to shorten this tests's runtime might be to try and
> > identify which of parallel_schedule tests leave objects behind and
> > create a shorter schedule with only those (a possible implementation
> > might keep a list of the slow tests that don't leave any useful object
> > behind, then filter parallel_schedule to exclude those; this ensures
> > test files created in the future are still used.)
>
> I'm not much a fan of approaches that require an extra schedule,
> because this is prone to forget the addition of objects that we'd want
> to cover for the scope of this thread with the dump/restore
> inter-dependencies, failing our goal of having more coverage.  And
> history has proven that we are quite bad at maintaining multiple
> schedules for the regression test suite (remember the serial one or
> the standby one in pg_regress?).  So we should really do things so as
> the schedules are down to a strict minimum: 1.

I see Alvaro's point about using a different and minimal schedule. We
already have 002_pg_upgrade and 027_stream_ as candidates which could
use schedules other than default and avoid wasting CPU cycles.
But I also agree with your opinion that maintaining multiple schedules
is painful and prone to errors.

What we could do is to create the schedule files automatically during
build. The automation script will require to know which file to place
in which schedules. That information could be either part of the sql
file itself or could be in a separate text file. For example, every
SQL file has the following line listing all the schedules that this
SQL file should be part of. E.g.

-- schedules: parallel, serial, upgrade

The automated script looks at every .sql file in a given sql directory
and creates the schedule files containing all the SQL files which had
respective schedules mentioned in their "schedule" annotation. The
automation script would flag SQL files that do not have scheduled
annotation so any new file added won't be missed. However, we will
still miss a SQL file if it wasn't part of a given schedule and later
acquired some changes which required it to be added to a new schedule.

If we go this route, we could make 'make check-tests' better. We could
add another annotation for depends listing all the SQL files that a
given SQL file depends upon. make check-tests would collect all
dependencies, sort them and run all the dependencies as well.

Of course that's out of scope for this patch. We don't have time left
for this in PG 18.

>
> If we're worried about the time taken by the test (spoiler: I am and
> the upgrade tests already show always as last to finish in parallel
> runs), I would recommend to put that under a PG_TEST_EXTRA.  I'm OK to
> add the switch to my buildfarm animals if this option is the consensus
> and if it gets into the tree.

I would prefer to run this test by default as Alvaro mentioned
previously. But if that means that we won't get this test committed at
all, I am ok putting it under PG_TEST_EXTRA. (Hence I have kept 0001
and 0002 separate.) But I will be disappointed if the test, which has
unearthed four bugs in a year alone, does not get committed to PG 18
because of this debate.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

28 March, 12:28:10

Vignesh and Alvaro

On Fri, Mar 28, 2025 at 12:02 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> >
> > Maybe
> > fail("roundtrip dump/restore of the regression database")
>
> No, that's losing some information like default installation and the
> same version.

How about "dump and restore across servers with same PostgreSQL
version using default installation". That's still mouthful but is more
readable.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Alvaro Herrera

Date:

28 March, 13:35:26

On 2025-Mar-28, Ashutosh Bapat wrote:

> No, that's losing some information like default installation and the
> same version.

You don't need to preserve such information.  This is just a test name.
People looking for more details can grep for the name and they will find
the comments.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

28 March, 14:20:54

On Fri, Mar 28, 2025 at 4:05 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-28, Ashutosh Bapat wrote:
>
> > No, that's losing some information like default installation and the
> > same version.
>
> You don't need to preserve such information.  This is just a test name.
> People looking for more details can grep for the name and they will find
> the comments.

Ok. In that case what's wrong with the testname I have in the patch?

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

28 March, 15:27:50

On Fri, Mar 28, 2025 at 12:20 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Fri, Mar 28, 2025 at 7:07 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Thu, Mar 27, 2025 at 06:15:06PM +0100, Alvaro Herrera wrote:
> > > BTW another idea to shorten this tests's runtime might be to try and
> > > identify which of parallel_schedule tests leave objects behind and
> > > create a shorter schedule with only those (a possible implementation
> > > might keep a list of the slow tests that don't leave any useful object
> > > behind, then filter parallel_schedule to exclude those; this ensures
> > > test files created in the future are still used.)
> >
> > I'm not much a fan of approaches that require an extra schedule,
> > because this is prone to forget the addition of objects that we'd want
> > to cover for the scope of this thread with the dump/restore
> > inter-dependencies, failing our goal of having more coverage.  And
> > history has proven that we are quite bad at maintaining multiple
> > schedules for the regression test suite (remember the serial one or
> > the standby one in pg_regress?).  So we should really do things so as
> > the schedules are down to a strict minimum: 1.
>
> I see Alvaro's point about using a different and minimal schedule. We
> already have 002_pg_upgrade and 027_stream_ as candidates which could
> use schedules other than default and avoid wasting CPU cycles.
> But I also agree with your opinion that maintaining multiple schedules
> is painful and prone to errors.
>
> What we could do is to create the schedule files automatically during
> build. The automation script will require to know which file to place
> in which schedules. That information could be either part of the sql
> file itself or could be in a separate text file. For example, every
> SQL file has the following line listing all the schedules that this
> SQL file should be part of. E.g.
>
> -- schedules: parallel, serial, upgrade
>
> The automated script looks at every .sql file in a given sql directory
> and creates the schedule files containing all the SQL files which had
> respective schedules mentioned in their "schedule" annotation. The
> automation script would flag SQL files that do not have scheduled
> annotation so any new file added won't be missed. However, we will
> still miss a SQL file if it wasn't part of a given schedule and later
> acquired some changes which required it to be added to a new schedule.
>
> If we go this route, we could make 'make check-tests' better. We could
> add another annotation for depends listing all the SQL files that a
> given SQL file depends upon. make check-tests would collect all
> dependencies, sort them and run all the dependencies as well.
>
> Of course that's out of scope for this patch. We don't have time left
> for this in PG 18.

I spent several hours today examining each SQL file to decide whether
or not it has "interesting" objects that it leaves behind for
dump/restore test. I came up with attached schedule - which may not be
accurate since I it would require much more time to examine all tests
to get an accurate schedule. But what I have got may be close enough.
With that we could save about 6 seconds on my laptop. If we further
compact the schedule reorganizing the parallel groups we may shave
some more seconds.

no modifications to parallel schedule
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.84s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.80s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
41.37s   28 subtests passed

with attached modified parallel schedule
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
36.13s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
35.86s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
36.33s   28 subtests passed
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK
36.02s   28 subtests passed

However, it's a very painful process to come up with the schedule and
more painful and error prone to maintain it. It could take many days
to come up with the right schedule which can become inaccurate the
moment next SQL file is added OR an existing file is modified to
add/drop "interesting" objects.

--
Best Wishes,
Ashutosh Bapat

Attachment

parallel_schedule_dump_restore

Re: Test to dump and restore objects left behind by regression

From

Alvaro Herrera

Date:

28 March, 17:11:15

On 2025-Mar-28, Ashutosh Bapat wrote:

> However, it's a very painful process to come up with the schedule and
> more painful and error prone to maintain it. It could take many days
> to come up with the right schedule which can become inaccurate the
> moment next SQL file is added OR an existing file is modified to
> add/drop "interesting" objects.

Hmm, I didn't mean that we'd maintain a separate schedule.  I meant that
we'd take the existing schedule, then apply some Perl magic to it that
grep-outs the tests that we know to contribute nothing, and generate a
new schedule file dynamically.  We don't need to maintain a separate
schedule file.

You're right that if an existing uninteresting test is modified to
create interesting objects, we'd lose coverage of those objects.  That
seems a much smaller problem to me.  So it's just a matter of doing some
Perl map/grep to generate a new schedule file using the attached
exclusion file.

(For what it's worth, what I did to try to determine which tests to
include, rather than scan each file manually, is to run pg_regress with
"test_setup thetest tablespace", then dump the regression database, and
see if anything is there that's not in the dump when I just with just
"test_setup tablespace".  I didn't carry the experiment to completion
though.)

For the future, we could annotate each test as you said, either by
adding a marker on the test file itself, or by adding something next to
its name in the schedule file, so the schedule file could look like:

test: plancache(dump_ignore) limit(stream_ignore) plpgsql copy2
    temp(stream_ignore,dump_ignore) domain rangefuncs(stream_ignore)
    prepare conversion truncate alter_table
    sequence polymorphism rowtypes returning largeobject with xml

... and so on.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Attachment

dump_roundtrip_exclude

Re: Test to dump and restore objects left behind by regression

From

Tom Lane

Date:

28 March, 17:22:08

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Hmm, I didn't mean that we'd maintain a separate schedule.  I meant that
> we'd take the existing schedule, then apply some Perl magic to it that
> grep-outs the tests that we know to contribute nothing, and generate a
> new schedule file dynamically.  We don't need to maintain a separate
> schedule file.

This seems like a fundamentally broken approach to me.

The entire argument for using the core regression tests as a source of
data to test dump/restore is that, more or less "for free", we can
expect to get coverage when new SQL language features are added.
That's always been a little bit questionable --- there's a temptation
to drop objects again at the end of a test script.  But with this,
it becomes a complete crapshoot whether the objects you need will be
included in the dump.

I think instead of going this direction, we really need to create a
separately-purposed script that simply creates "one of everything"
without doing anything else (except maybe loading a little data).
I believe it'd be a lot easier to remember to add to that when
inventing new SQL than to remember to leave something behind from the
core regression tests.  This would also be far faster to run than any
approach that involves picking a random subset of the core test
scripts.

            regards, tom lane

Re: Test to dump and restore objects left behind by regression

From

Alvaro Herrera

Date:

28 March, 21:12:58

On 2025-Mar-28, Tom Lane wrote:

> I think instead of going this direction, we really need to create a
> separately-purposed script that simply creates "one of everything"
> without doing anything else (except maybe loading a little data).
> I believe it'd be a lot easier to remember to add to that when
> inventing new SQL than to remember to leave something behind from the
> core regression tests.  This would also be far faster to run than any
> approach that involves picking a random subset of the core test
> scripts.

FWIW this sounds closely related to what I tried to do with
src/test/modules/test_ddl_deparse; it's currently incomplete, but maybe
we can use that as a starting point.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Always assume the user will do much worse than the stupidest thing
you can imagine."                                (Julien PUYDT)

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

31 March, 14:37:15

On Fri, Mar 28, 2025 at 11:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-28, Tom Lane wrote:
>
> > I think instead of going this direction, we really need to create a
> > separately-purposed script that simply creates "one of everything"
> > without doing anything else (except maybe loading a little data).
> > I believe it'd be a lot easier to remember to add to that when
> > inventing new SQL than to remember to leave something behind from the
> > core regression tests.  This would also be far faster to run than any
> > approach that involves picking a random subset of the core test
> > scripts.

It's easier to remember to do something or not do something in the
same file than in some other file. I find it hard to believe that
introducing another set of SQL files somewhere far from regress would
make this problem easier.

The number of states in which objects can be left behind in the
regress/sql is very large - and maintaining that 1:1 in some other set
of scripts is impossible unless it's automated.

> FWIW this sounds closely related to what I tried to do with
> src/test/modules/test_ddl_deparse; it's currently incomplete, but maybe
> we can use that as a starting point.

create_table.sql in test_ddl_deparse has only one statement creating
an inheritance table whereas there are dozens of different states of
parent/child tables created by regress. It will require a lot of work
to bridge the gap between regress_ddl_deparse and regress and more
work to maintain it.

I might be missing something in your ideas.

IMO, whatever we do it should rely on a single set of files. One
possible way could be to break the existing files into three files
each, containing DDL, DML and queries from those files respectively
and create three schedules DDL, DML and queries containing the
respective files. These schedules will be run as required. Standard
regression run runs all the three schedules one by one. But
002_pg_upgrade will run DDL and DML on the source database and run
queries on target - thus checking sanity of the dump/restore or
pg_upgrade beyond just the dump comparison. 027_stream_regress might
run DDL, DML on the source server and queries on the target.

But that too is easier said than done for:
1. Our tests mix all three kinds of statements and also rely on the
order in which they are run. It will require some significant effort
to carefully separate the statements.
2. With the new set of files backpatching would become hard.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

31 March, 14:54:03

On Mon, Mar 31, 2025 at 5:07 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
The bug related to materialized views has been fixed and now the test
passes even if we compare statistics from dumped and restored
databases. Hence removing 0003. In the attached patchset I have also
addressed Vignesh's below comment

On Thu, Mar 27, 2025 at 10:01 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Mar 27, 2025 at 6:01 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > 2) Should "`" be ' or " here, we   generally use "`" to enclose commands:
> > +# It is expected that regression tests, which create `regression` database, are
> > +# run on `src_node`, which in turn, is left in running state. A fresh node is
> > +# created using given `node_params`, which are expected to be the
> > same ones used
> > +# to create `src_node`, so as to avoid any differences in the databases.
>
> Looking at prologues or some other functions, I see that we don't add
> any decoration around the name of the argument. Hence dropped ``
> altogether. Will post it with the next set of patches.

--
Best Wishes,
Ashutosh Bapat

On Tue, Apr 1, 2025 at 11:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-31, Daniel Gustafsson wrote:
>
> > Given where we are in the cycle, it seems to make sense to stick to using the
> > schedule we already have rather than invent a new process for generating it,
> > and work on that for 19?
>
> No objections to that.  I'll see about getting this committed during my
> morning today, so that I have plenty of time to watch the buildfarm.

Thanks Alvaro.
Just today morning, I found something which looks like another bug in
statistics dump/restore [1]. As Daniel has expressed upthread [2], we
should go ahead and commit the test even if the bug is not fixed. But
in case it creates a lot of noise and makes the build farm red, we
could suppress the failure by not dumping statistics for comparison
till the bug is fixed. PFA patchset which reintroduces 0003 which
suppresses the statistics dump - in case we think it's needed. I have
made some minor cosmetic changes to 0001 and 0002 as well.

I will also watch buildfarm too, once you commit the patch.

[1] https://www.postgresql.org/message-id/CAExHW5sFOgcUkVtZ8=QCAE+jv=sbNdBKq0xZCNJTh7019ZM+CQ@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat

On Wed, Apr 2, 2025 at 3:36 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> >
> > No commitfest entry please.  Better to add an open item on the wiki
> > page.
> > https://wiki.postgresql.org/wiki/Open_Items
>
> Posted it on the thread where I have reported the bug. Hopefully, we
> will commit both the bug fix and test change to enable stats together.

Looks like the problem is in the test itself as pointed out by Jeff in
[1]. PFA patch fixing the test and enabling statistics back.

The test file is arranged as follows
1. Setup old cluster (this step also runs regression if needed)
2. create new cluster for upgrade by modifying some configuration from
the old cluster.
3. disable autovacuum on old cluster
4. Run dump/restore roundtrip test which creates a destination cluster
with the same configuration as the old cluster

A note about variable name changes and introduction of new variables.
We run step 2 between 1 and 3 so that autovacuum gets a chance to run
on the old cluster and update statistics. Autovacuum run is not
necessary but useful here. Before these changes all the cluster
initializations were using the same variables @initdb_params and
%node_params. However with these changes, we modify the variable in
step 2 and then again require original values in step 4. So I have
used two sets of variables prefixed with old_ and new_ for clusters
created in 1st step and 2nd step respectively. 4th step uses the
variables with prefix old_. I think this change eliminates confusion
caused by using same variables with different values.

[1] https://www.postgresql.org/message-id/5f3703fd7f27da62a8f3615218f937507f522347.camel%40j-davis.com

I will watch CF CI run to see if we see difference in statistics even
after this change.

--
Best Wishes,
Ashutosh Bapat

Attachment

0001-Fix-differences-in-dumped-statistics-20250403.patch

Re: Test to dump and restore objects left behind by regression

From

Alvaro Herrera

Date:

03 April, 11:20:09

On 2025-Apr-03, Ashutosh Bapat wrote:

> Looks like the problem is in the test itself as pointed out by Jeff in
> [1]. PFA patch fixing the test and enabling statistics back.

Thanks, pushed.

> A note about variable name changes and introduction of new variables.
> We run step 2 between 1 and 3 so that autovacuum gets a chance to run
> on the old cluster and update statistics. Autovacuum run is not
> necessary but useful here. Before these changes all the cluster
> initializations were using the same variables @initdb_params and
> %node_params. However with these changes, we modify the variable in
> step 2 and then again require original values in step 4. So I have
> used two sets of variables prefixed with old_ and new_ for clusters
> created in 1st step and 2nd step respectively. 4th step uses the
> variables with prefix old_. I think this change eliminates confusion
> caused by using same variables with different values.

This was a good change, thanks.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No es bueno caminar con un hombre muerto"

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

03 April, 12:14:59

On Thu, Apr 3, 2025 at 1:50 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Apr-03, Ashutosh Bapat wrote:
>
> > Looks like the problem is in the test itself as pointed out by Jeff in
> > [1]. PFA patch fixing the test and enabling statistics back.
>
> Thanks, pushed.

Thanks.

--
Best Wishes,
Ashutosh Bapat

Re: Test to dump and restore objects left behind by regression

From

Andres Freund

Date:

03 April, 16:40:49

Hi,

On 2025-04-03 10:20:09 +0200, Alvaro Herrera wrote:
> On 2025-Apr-03, Ashutosh Bapat wrote:
>
> > Looks like the problem is in the test itself as pointed out by Jeff in
> > [1]. PFA patch fixing the test and enabling statistics back.
>
> Thanks, pushed.

Since then the pg_upgrade tests have been failing on skink/valgrind, due to
exceeding the already substantially increased timeout.

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=skink&dt=2025-04-03%2007%3A06%3A19&stg=pg_upgrade-check
(note that there are other issues in that run)

284/333 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade                               TIMEOUT        10000.66s
killedby signal 15 SIGTERM

[10:38:19.815](16.712s) ok 20 - check that locales in new cluster match original cluster
...
# Running: pg_dumpall --no-sync --dbname port=15114 host=/tmp/bh_AdT5uvQ dbname='postgres' --file
/home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/pg_upgrade/002_pg_upgrade/data/tmp_test_gp2G/dump2.sql
death by signal at /home/bf/bf-build/skink-master/HEAD/pgsql/src/test/perl/PostgreSQL/Test/Cluster.pm line 181.
...
[10:44:11.720](351.905s) # Tests were run but no plan was declared and done_testing() was not seen.

I've increased the timeout even further, but I can't say that I am happy about
the slowest test getting even slower. Adding test time in the serially slowest
test is way worse than adding the same time in a concurrent test.

I suspect that the test will go a bit faster if log_statement weren't forced
on, printing that many log lines, with context, does make valgrind slower,
IME. But Cluster.pm forces it to on, and I suspect that putting a global
log_statement=false into TEMP_CONFIG would have it's own disadvantages.

/me and checks prices for increasing the size of skink's host.

Greetings,

Andres

Re: Test to dump and restore objects left behind by regression

From

Alvaro Herrera

Date:

03 April, 20:14:02

On 2025-Apr-03, Andres Freund wrote:

> I've increased the timeout even further, but I can't say that I am happy about
> the slowest test getting even slower. Adding test time in the serially slowest
> test is way worse than adding the same time in a concurrent test.

Yeah.  We discussed strategies to shorten the runtime, but the agreement
upthread was that we'd look for more elaborate ways to do that
afterwards.  As I mentioned, I can see adding something like
PG_TEST_EXCLUDE that we could use to suppress this test on slow hosts.
Would that work for you?

(We also discussed the fact that this was part of 002_pg_upgrade.pl
instead of being elsewhere.  The reason is that this depends on the
regression tests having run, and this is the only TAP test that does
that.   Well, this one and 027_stream_regress.pl which is even slower.)

> I suspect that the test will go a bit faster if log_statement weren't forced
> on, printing that many log lines, with context, does make valgrind slower,
> IME. But Cluster.pm forces it to on, and I suspect that putting a global
> log_statement=false into TEMP_CONFIG would have it's own disadvantages.

I'm sure we can make this change as well somehow, overridding the
setting just 002_pg_upgrade.pl, as attached.  I don't think it's
relevant for this particular test.  The log files go from 21 MB to
2.4 MB.  It's not nothing ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Selbst das größte Genie würde nicht weit kommen, wenn es
alles seinem eigenen Innern verdanken wollte." (Johann Wolfgang von Goethe)
               Ni aún el genio más grande llegaría muy lejos si
                    quisiera sacarlo todo de su propio interior.

Attachment

nologstatements.patch

Re: Test to dump and restore objects left behind by regression

From

Ashutosh Bapat

Date:

04 April, 14:11:51

On Thu, Apr 3, 2025 at 10:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Apr-03, Andres Freund wrote:
>
> > I've increased the timeout even further, but I can't say that I am happy about
> > the slowest test getting even slower. Adding test time in the serially slowest
> > test is way worse than adding the same time in a concurrent test.
>
> Yeah.  We discussed strategies to shorten the runtime, but the agreement
> upthread was that we'd look for more elaborate ways to do that
> afterwards.  As I mentioned, I can see adding something like
> PG_TEST_EXCLUDE that we could use to suppress this test on slow hosts.
> Would that work for you?
>
> (We also discussed the fact that this was part of 002_pg_upgrade.pl
> instead of being elsewhere.  The reason is that this depends on the
> regression tests having run, and this is the only TAP test that does
> that.   Well, this one and 027_stream_regress.pl which is even slower.)
>
> > I suspect that the test will go a bit faster if log_statement weren't forced
> > on, printing that many log lines, with context, does make valgrind slower,
> > IME. But Cluster.pm forces it to on, and I suspect that putting a global
> > log_statement=false into TEMP_CONFIG would have it's own disadvantages.
>
> I'm sure we can make this change as well somehow, overridding the
> setting just 002_pg_upgrade.pl, as attached.  I don't think it's
> relevant for this particular test.  The log files go from 21 MB to
> 2.4 MB.  It's not nothing ...

It doesn't show any time improvement on my laptop, but it may improve
valgrind timing. My valgrind setup is broken, trying to fix it and run
it. I have included this as 0002 in the attached patchset.

0001 is an attempt to reduce runtime of the test by not setting up a
cluster for restoring the database. Instead the test uses the upgraded
node as the target. This works well since we expect the old node and
new node to be running the same version and default install. The only
unpleasantness is 1. dump and restore phases are spatially and
temporally separated 2. The upgraded regression database needs to be
renamed to save its state for diagnosis, if required. But as a result
this saves 3 seconds on my laptop. Earlier we saw that the test added
9 seconds on my laptop and we gained back 3 seconds; doesn't seem bad.
It will show a significant difference in valgrind run.

--
Best Wishes,
Ashutosh Bapat

Hi,

On 2025-04-04 12:01:16 -0400, Andres Freund wrote:
> FWIW, for me 027 is actually considerably faster. In an cassert -O0 build (my
> normal development env, I find even -Og too problematic for debugging):
> 
> pg_upgrade/002_pg_upgrade    96.61s
> recovery/027_stream_regress  66.04s
> 
> After
>   git revert 8806e4e8deb1e21715e031e17181d904825a410e abe56227b2e213755dd3e194c530f5f06467bd7c
172259afb563d35001410dc6daad78b250924038
> 
> pg_upgrade/002_pg_upgrade    75.09s
> 
> Slower by 29%, far from the 3s increased time I saw mentioned somewhere.
> 
> 
> And this really affects the overall test time:
> 
> All tests before:
>     real    1m38.173s
>     user    5m52.500s
>     sys    4m23.574s
> 
> All tests after:
>     real    2m0.397s
>     user    5m53.625s
>     sys    4m30.518s
> 
> The CPU time increase is rather minimal, but the wall clock time increase is
> 22%.
> 
> 17:
>     real    1m14.822s
>     user    4m2.630s
>     sys    3m22.384s
> 
> We regressed wall clock time *60%* from 17->18. Some test cycle increase is
> reasonable and can largely be compensated with hardware, but this cycle we're
> growing way faster than hardware gets faster.  I don't think that's
> sustainable.

FWIW, with cassert and -O2, it's:

17:
    real    0m53.981s
    user    3m22.837s
    sys    3m24.237s

HEAD:
    real    1m19.749s
    user    4m54.526s
    sys    4m15.657s

so this isn't just due to me using -O0. A 48% increase is better than a 60%
increase, but it's still not sustainable.

Greetings,

Andres Freund