Re: A test for replay of regression tests

Hi,

Rebased and updated based on feedback. Responses to multiple emails below:

On Thu, Dec 16, 2021 at 1:22 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Wed, Dec 15, 2021 at 05:50:45PM +0900, Michael Paquier wrote:
> > Hmm. FWIW, I am working on doing similar for pg_upgrade to switch to
> > TAP there, and we share a lot in terms of running pg_regress on an
> > exising cluster. Wouldn't it be better to move this definition to
> > src/Makefile.global.in rather than src/test/recovery/?
> >
> > My pg_regress command is actually very similar to yours, so I am
> > wondering if this would be better if somehow centralized, perhaps in
> > Cluster.pm.

Thanks for looking. Right, it sounds like you'll have the same
problems I ran into. I haven't updated this patch for that yet, as
I'm not sure exactly what you need and we could easily move it in a
later commit. Does that seem reasonable?

> By the way, while I was sorting out my things, I have noticed that v4
> does not handle EXTRA_REGRESS_OPT. Is that wanted? You could just
> add that into your patch set and push the extra options to the
> pg_regress command:
> my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
> my @extra_opts = split(/\s+/, $extra_opts_val);

Seems like a good idea for consistency, but isn't that a variable
that's supposed to be expanded by a shell, not naively split on
whitespace? Perhaps we should use the single-argument variant of
system(), so the whole escaped enchilada is passed to a shell? Tried
like that in this version (though now I'm wondering what the correct
perl incantation is to shell-escape $outputdir and $dlpath...)

On Sat, Dec 11, 2021 at 1:17 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2021-12-10 12:58:01 +1300, Thomas Munro wrote:
> > +$node_primary->adjust_conf('postgresql.conf', 'max_connections', '25', 1);
>
> Possible that this will cause problem on some *BSD platform with a limited
> count of semaphores. But we can deal with that if / when it happens.

Right, those systems don't work out of the box for us already without
sysctl tweaks, so it doesn't matter if animals have to be adjusted
further.

> > @@ -590,16 +595,35 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
> > char *linkloc;
> > char *location_with_version_dir;
> > struct stat st;
> > + bool in_place;
> >
> > linkloc = psprintf("pg_tblspc/%u", tablespaceoid);
> > - location_with_version_dir = psprintf("%s/%s", location,
> > +
> > + /*
> > + * If we're asked to make an 'in place' tablespace, create the directory
> > + * directly where the symlink would normally go. This is a developer-only
> > + * option for now, to facilitate regression testing.
> > + */
> > + in_place =
> > + (allow_in_place_tablespaces || InRecovery) && strlen(location) == 0;
>
> Why is in_place set to true by InRecovery?

Well the real condition is strlen(location) == 0, and the other part
is a sort of bit belt-and-braces check, but yeah, I should just remove
that part. Done.

> ISTM that allow_in_place_tablespaces should be checked in CreateTableSpace(),
> and create_tablespace_directories() should just do whatever it's told?
> Otherwise it seems there's ample potential for confusion, e.g. because of the
> GUC differing between primary and replica, or between crash and a new start.

Agreed, that was the effect but the extra unnecessary check was a bit confusing.

> > /*
> > * Attempt to coerce target directory to safe permissions. If this fails,
> > * it doesn't exist or has the wrong owner.
> > */
> > - if (chmod(location, pg_dir_create_mode) != 0)
> > + if (!in_place && chmod(location, pg_dir_create_mode) != 0)
> > {
> > if (errno == ENOENT)
> > ereport(ERROR,
>
> Maybe add a coment saying that we don't need to chmod here because
> MakePGDirectory() takes care of that?

Done.

> > @@ -648,13 +672,13 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
> > /*
> > * In recovery, remove old symlink, in case it points to the wrong place.
> > */
> > - if (InRecovery)
> > + if (!in_place && InRecovery)
> > remove_tablespace_symlink(linkloc);
>
> I don't think it's right to check !in_place as you do here, given that it
> currently depends on a GUC setting that's possibly differs between WAL
> generation and replay time?

I have to, because otherwise we'll remove the directory we just
created at the top of the function. It doesn't really depend on a GUC
(clearer after previous change).

> Perhaps also add a test that we catch "in-place" tablespace creation with
> allow_in_place_tablespaces = false? Although perhaps that's better done in the
> previous commit...

There was a test for that already, see this bit:

+-- empty tablespace locations are not usually allowed
+CREATE TABLESPACE regress_tblspace LOCATION ''; -- fail
+ERROR: tablespace location must be an absolute path

> > +++ b/src/test/modules/test_misc/t/002_tablespace.pl
>
> Two minor things that I think would be worth testing here:
> 1) moving between two "absolute" tablespaces. That could conceivably break differently
> between in-place and relative tablespaces.
> 2) Moving between absolute and relative tablespace.

Done.

> > +# required for 027_stream_regress.pl
> > +REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/test/recovery
> > +export REGRESS_OUTPUTDIR
>
> Why do we need this?

The Make macro "prove_check" (src/Makefile.global.in) always changes
to the source directory to run TAP tests. Without an explicit
directive to control where regression test output goes, it got
splattered all over the source tree in VPATH builds. I didn't see an
existing way to adjust that (did I miss something?). Hence desire to
pass down a path in the build tree. Better ideas welcome.

> > +# Initialize primary node
> > +my $node_primary = PostgreSQL::Test::Cluster->new('primary');
> > +$node_primary->init(allows_streaming => 1);
> > +$node_primary->adjust_conf('postgresql.conf', 'max_connections', '25', 1);
>
> Probably should set at least max_prepared_transactions > 0, so the tests
> requiring prepared xacts can work. They have nontrivial replay routines, so
> that seems worthwhile?

Good idea. Done.

> > +# Perform a logical dump of primary and standby, and check that they match
> > +command_ok(
> > + [ 'pg_dump', '-f', $outputdir . '/primary.dump', '--no-sync',
> > + '-p', $node_primary->port, 'regression' ],
> > + 'dump primary server');
> > +command_ok(
> > + [ 'pg_dump', '-f', $outputdir . '/standby.dump', '--no-sync',
> > + '-p', $node_standby_1->port, 'regression' ],
> > + 'dump standby server');
> > +command_ok(
> > + [ 'diff', $outputdir . '/primary.dump', $outputdir . '/standby.dump' ],
> > + 'compare primary and standby dumps');
> > +
> > +$node_standby_1->stop;
> > +$node_primary->stop;
>
> This doesn't verify if global objects are replayed correctly. Perhaps it'd be
> better to use pg_dumpall?

Good idea. Done.