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

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(at)paquier(dot)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