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.mu...@gmail.com> wrote:
> > On Fri, Dec 10, 2021 at 8:38 AM Andres Freund <and...@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
