On Tue, Mar 28, 2023 at 05:15:25PM +1300, Thomas Munro wrote:
> The commit message explains pretty well, and it seems to work in
> simple testing, and yeah commit c6f2f016 was not a work of art.
> pg_basebackeup --format=plain "worked", but your way is better. I
> guess we should add a test of -Fp too, to keep it working? Here's one
> of those.
The patch is larger than it is complicated. Particularly, in xlog.c,
this is just adding one block for the PGFILETYPE_DIR case to store a
relative path.
> I know it's not your patch's fault, but I wonder if this might break
> something. We have this strange beast ti->rpath (commit b168c5ef in
> 2014):
>
> + /*
> + * Relpath holds the relative path of the tablespace directory
> + * when it's located within PGDATA, or NULL if it's located
> + * elsewhere.
> + */
>
> That's pretty confusing, because relative paths have been banned since
> the birth of tablespaces (commit 2467394ee15, 2004):
I think that this comes down to people manipulating pg_tblspc/ by
themselves post-creation with CREATE, because we don't track the
location anywhere post-creation and rely on the structure of
pg_tblspc/ for any future decision taken by the backend? I recall
this specific case, where users were creating tablespaces in PGDATA
itself to make "cleaner" for them the structure in the data folder,
even if it makes no sense as the mount point is the same.. 33cb8ff
added a warning about that in tablespace.c, but we could be more
aggressive and outright drop this case entirely when in-place
tablespaces are not involved.
Tablespace maps defined in pg_basebackup -T require both the old and
new locations to be absolute, so it seems shaky to me to assume that
this should always be fine in the backend..
> I guess what I'm wondering here is if there is a hazard where we
> confuse these outlawed tablespaces that supposedly roam the plains
> somewhere in your code that is assuming that "relative implies
> in-place". Or if not now, maybe in future changes. I wonder if these
> "semi-supported-but-don't-tell-anyone" relative symlinks are worthy of
> a defensive test (or is it in there somewhere already?).
Yeah, it makes for surprising and sometimes undocumented behaviors,
which is confusing for users and developers at the end. I'd like to
think that we'd live happier long-term if the borders are clearer, aka
switch to more aggressive ERRORs instead of WARNINGs in some places
where the boundaries are blurry. A good thing about in-place
tablespaces taken in isolation is that the borders of what you can do
are well-defined, which comes down to the absolute vs relative path
handling.
Looking at the patch, nothing really stands out..
--
Michael
