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

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