Re: Test to dump and restore objects left behind by regression - Mailing list pgsql-hackers

On Tue, 25 Mar 2025 at 16:09, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Mon, Mar 24, 2025 at 5:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2025-Mar-24, Ashutosh Bapat wrote:
> >
> > > One concern I have with directory format is the dumped database is not
> > > readable. This might make investigating a but identified the test a
> > > bit more complex.
> >
> > Oh, it's readable all right.  You just need to use `pg_restore -f-` to
> > read it.  No big deal.
> >
> >
> > So I ran this a few times:
> > /usr/bin/time make -j8 -Otarget -C /pgsql/build/master check-world -s PROVE_FLAGS="-c -j6" > /dev/null
> >
> > commenting out the call to test_regression_dump_restore() to test how
> > much additional runtime does the new test incur.
> >
> > With test:
> >
> > 136.95user 116.56system 1:13.23elapsed 346%CPU (0avgtext+0avgdata 250704maxresident)k
> > 4928inputs+55333008outputs (114major+14784937minor)pagefaults 0swaps
> >
> > 138.11user 117.43system 1:15.54elapsed 338%CPU (0avgtext+0avgdata 278592maxresident)k
> > 48inputs+55333464outputs (80major+14794494minor)pagefaults 0swaps
> >
> > 137.05user 113.13system 1:08.19elapsed 366%CPU (0avgtext+0avgdata 279272maxresident)k
> > 48inputs+55330064outputs (83major+14758028minor)pagefaults 0swaps
> >
> > without the new test:
> >
> > 135.46user 114.55system 1:14.69elapsed 334%CPU (0avgtext+0avgdata 145372maxresident)k
> > 32inputs+55155256outputs (105major+14737549minor)pagefaults 0swaps
> >
> > 135.48user 114.57system 1:09.60elapsed 359%CPU (0avgtext+0avgdata 148224maxresident)k
> > 16inputs+55155432outputs (95major+14749502minor)pagefaults 0swaps
> >
> > 133.76user 113.26system 1:14.92elapsed 329%CPU (0avgtext+0avgdata 148064maxresident)k
> > 48inputs+55154952outputs (84major+14749531minor)pagefaults 0swaps
> >
> > 134.06user 113.83system 1:16.09elapsed 325%CPU (0avgtext+0avgdata 145940maxresident)k
> > 32inputs+55155032outputs (83major+14738602minor)pagefaults 0swaps
> >
> > The increase in duration here is less than a second.
> >
> >
> > My conclusion with these numbers is that it's not worth hiding this test
> > in PG_TEST_EXTRA.
>
> Thanks for the conclusion.
>
> On Mon, Mar 24, 2025 at 3:29 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> >
> > > On 24 Mar 2025, at 10:54, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > > 0003 - same as 0002 in the previous patch set. It excludes statistics
> > > from comparison, otherwise the test will fail because of bug reported
> > > at [1]. Ideally we shouldn't commit this patch so as to test
> > > statistics dump and restore, but in case we need the test to pass till
> > > the bug is fixed, we should merge this patch to 0001 before
> > > committing.
> >
> > If the reported bug isn't fixed before feature freeze I think we should commit
> > this regardless as it has clearly shown value by finding bugs (though perhaps
> > under PG_TEST_EXTRA or in some disconnected till the bug is fixed to limit the
> > blast-radius in the buildfarm).
>
> Combining Alvaro's and Daniel's recommendations, I think we should
> squash all the three of my patches while committing the test if the
> bug is not fixed by then. Otherwise we should squash first two patches
> and commit it. Just attaching the patches again for reference.

Couple of minor thoughts:
1) I felt this error message is not conveying the error message correctly:
+       if ($src_node->pg_version != $dst_node->pg_version
+               or defined $src_node->{_install_path})
+       {
+               fail("same version dump and restore test using default
installation");
+               return;
+       }

how about something like below:
fail("source and destination nodes must have the same PostgreSQL
version and default installation paths");

2) Should "`" be ' or " here, we   generally use "`" to enclose commands:
+# It is expected that regression tests, which create `regression` database, are
+# run on `src_node`, which in turn, is left in running state. A fresh node is
+# created using given `node_params`, which are expected to be the
same ones used
+# to create `src_node`, so as to avoid any differences in the databases.

There are few other instances similarly in the file.

Regards,
Vignesh