> 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
