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
signature.asc
Description: PGP signature