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

On Wed, Dec 18, 2024 at 7:39 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 18 Dec 2024, at 12:28, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)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".

Done.

>
>
> + 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.

>
> -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.
>

In one of the earlier versions of the patch, there was
filter_dump_for_regress or some such function which was used to filter
the dump from the regression database. Name was changed to
differentiate between the two functions. But the new function is now
named as adjust_regress_dumpfile() so this name change is not required
anymore. Reverting it. I have left the comment change since the test
file now has tests for both upgrade and dump/restore.

>
> + 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.

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

I didn't see this being used with other open calls in the file. For
that matter we are not using $! with open() in many test files. But it
seems useful. Done

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

Thanks for pointing out. Fixed.

>
> + 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.

Good suggestion. Done.

0001 - same as 0001 from previous version
0002 - addresses above comments

--
Best Wishes,
Ashutosh Bapat