Re: A test for replay of regression tests

Hi,

On 2021-12-10 12:58:01 +1300, Thomas Munro wrote:
> > What's the relation of this to the rest?
> 
> Someone decided that allow_streaming should imply max_connections =
> 10, but we need ~20 to run the parallel regression test schedule.
> However, I can just as easily move that to a local adjustment in the
> TAP test file.  Done, like so:
> 
> +$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.



> > Separately: I think the case of seeing diffs will be too hard to debug like
> > this, as the difference isn't shown afaict?
> 
> Seems to be OK.  The output goes to
> src/test/recovery/tmp_check/log/regress_log_027_stream_regress, so for
> example if you comment out the bit that deals with SEQUENCE caching
> you'll see:

Ah, ok. Not sure what I thought there...


> On Fri, Dec 10, 2021 at 10:35 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > On Fri, Dec 10, 2021 at 8:38 AM Andres Freund <andres@anarazel.de> wrote:
> > > Personally I'd rather put relative tablespaces into a dedicated directory or
> > > just into pg_tblspc, but without a symlink. Some tools need to understand
> > > tablespace layout etc, and having them in a directory that, by the name, will
> > > also contain other things seems likely to cause confusion.
> 
> Ok, in this version I have a developer-only GUC
> allow_in_place_tablespaces instead.  If you turn it on, you can do:
> 
> CREATE TABLESPACE regress_blah LOCATION = '';

> ... and then pg_tblspc/OID is created directly as a directory.  Not
> allowed by default or documented.

WFM. I think we might eventually want to allow it by default, but we can deal
with that whenever somebody wants to spend the energy doing so.



> @@ -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?

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.


> +    if (in_place)
> +    {
> +        if (MakePGDirectory(linkloc) < 0 && errno != EEXIST)
> +            ereport(ERROR,
> +                    (errcode_for_file_access(),
> +                     errmsg("could not create directory \"%s\": %m",
> +                            linkloc)));
> +    }
> +
> +    location_with_version_dir = psprintf("%s/%s", in_place ? linkloc : location,
>                                           TABLESPACE_VERSION_DIRECTORY);
>  
>      /*
>       * 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?


> @@ -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?


> --- a/src/test/regress/output/tablespace.source
> +++ b/src/test/regress/expected/tablespace.out
> @@ -1,7 +1,18 @@
> +-- relative tablespace locations are not allowed
> +CREATE TABLESPACE regress_tblspace LOCATION 'relative'; -- fail
> +ERROR:  tablespace location must be an absolute path
> +-- empty tablespace locations are not usually allowed
> +CREATE TABLESPACE regress_tblspace LOCATION ''; -- fail
> +ERROR:  tablespace location must be an absolute path
> +-- as a special developer-only option to allow us to use tablespaces
> +-- with streaming replication on the same server, an empty location
> +-- can be allowed as a way to say that the tablespace should be created
> +-- as a directory in pg_tblspc, rather than being a symlink
> +SET allow_in_place_tablespaces = true;
>  -- create a tablespace using WITH clause
> -CREATE TABLESPACE regress_tblspacewith LOCATION '@testtablespace@' WITH (some_nonexistent_parameter = true); --
fail
> +CREATE TABLESPACE regress_tblspacewith LOCATION '' WITH (some_nonexistent_parameter = true); -- fail
>  ERROR:  unrecognized parameter "some_nonexistent_parameter"
> -CREATE TABLESPACE regress_tblspacewith LOCATION '@testtablespace@' WITH (random_page_cost = 3.0); -- ok
> +CREATE TABLESPACE regress_tblspacewith LOCATION '' WITH (random_page_cost = 3.0); -- ok

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...


> +++ 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.


> +# required for 027_stream_regress.pl
> +REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/test/recovery
> +export REGRESS_OUTPUTDIR

Why do we need this?


> +# 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?


> +# 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?

Greetings,

Andres Freund