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?
