Re: allow_in_place_tablespaces vs. pg_basebackup

From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: allow_in_place_tablespaces vs. pg_basebackup
Date: 2023-03-28 04:15:25
Message-ID: CA+hUKGJhLNV_AKp9fZ=H_Cr5SBkMS-Ynhd2Ro=EHCSobj4Z9NA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

The commit message explains prettty 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.

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):

+ /*
+ * Allowing relative paths seems risky
+ *
+ * this also helps us ensure that location is not empty or whitespace
+ */
+ if (!is_absolute_path(location))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("tablespace location must be
an absolute path")));

The discussion that produced the contradiction:
https://www.postgresql.org/message-id/flat/m2ob3vl3et.fsf%402ndQuadrant.fr

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

Originally when trying to implement allow_in_place_tablespaces, I
instead tried allowing limited relative tablespaces, so you could use
LOCATION 'pg_tblspc/my_directory', and then it would still create a
symlink 1234 -> my_directory, which probably would have All Just
Worked™ given the existing ti->rpath stuff, and possibly made the
users that thread was about happy, but I had to give that up because
it didn't work on Windows (no relative symlinks). Oh well.

Attachment Content-Type Size
0001-Fix-pg_basebackup-with-in-place-tablespaces-some-mor.patch text/x-patch 11.7 KB
0002-fixup.patch text/x-patch 2.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Hayato Kuroda (Fujitsu) 2023-03-28 04:19:04 RE: PGdoc: add missing ID attribute to create_subscription.sgml
Previous Message houzj.fnst@fujitsu.com 2023-03-28 04:12:34 RE: Support logical replication of DDLs