Regression test PANICs with master-standby setup on same machine

Hello.

At Mon, 22 Apr 2019 15:52:59 +0530, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote in <CAGz5QC+j1BDq7onp6H8Cye-ahD2zS1dLttp-dEuEoZStEjxq5Q@mail.gmail.com>

Hello hackers,

With a master-standby setup configured on the same machine, I'm
getting a panic in tablespace test while running make installcheck.

1. CREATE TABLESPACE regress_tblspacewith LOCATION 'blah';
2. DROP TABLESPACE regress_tblspacewith;
3. CREATE TABLESPACE regress_tblspace LOCATION 'blah';
-- do some operations in this tablespace
4. DROP TABLESPACE regress_tblspace;

The master panics at the last statement when standby has completed
applying the WAL up to step 2 but hasn't started step 3.
PANIC: could not fsync file
"pg_tblspc/16387/PG_12_201904072/16384/16446": No such file or
directory

The reason is both the tablespace points to the same location. When
master tries to delete the new tablespace (and requests a checkpoint),
the corresponding folder is already deleted by the standby while
applying WAL to delete the old tablespace. I'm able to reproduce the
issue with the attached script.

sh standby-server-setup.sh
make installcheck

I accept that configuring master-standby on the same machine for this
test is not okay. But, can we avoid the PANIC somehow? Or, is this
intentional and I should not include testtablespace in this case?

If you don't have a problem using TAP test suite, tablespace is
allowed with a bit restricted steps using the first patch in my
just posted patchset[1]/messages/by-id/20190422.211933.156769089.horiguchi.kyotaro@lab.ntt.co.jp. This will work for you if you are okay
with creating a standby after creating a tablespace. See the
second patch in the patchset.

If you stick on shell script, the following steps allow tablespaces.

1. Create tablespace directories for both master and standby.
2. Create a master then start.
3. Create tablespaces on the master.
4. Create a standby using pg_basebackup --tablespace_mapping=<mstdir>=<sbydir>
5. Start the standby.

[1]: /messages/by-id/20190422.211933.156769089.horiguchi.kyotaro@lab.ntt.co.jp

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Apr 22, 2019 at 6:07 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

If you don't have a problem using TAP test suite, tablespace is
allowed with a bit restricted steps using the first patch in my
just posted patchset[1]. This will work for you if you are okay
with creating a standby after creating a tablespace. See the
second patch in the patchset.

If you stick on shell script, the following steps allow tablespaces.

1. Create tablespace directories for both master and standby.
2. Create a master then start.
3. Create tablespaces on the master.
4. Create a standby using pg_basebackup --tablespace_mapping=<mstdir>=<sbydir>
5. Start the standby.

[1] /messages/by-id/20190422.211933.156769089.horiguchi.kyotaro@lab.ntt.co.jp

Thank you for the info. I'll try the same.

--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 22, 2019 at 03:52:59PM +0530, Kuntal Ghosh wrote:

I accept that configuring master-standby on the same machine for this
test is not okay. But, can we avoid the PANIC somehow? Or, is this
intentional and I should not include testtablespace in this case?

Well, it is a bit more than "not okay", as the primary and the
standby step on each other's toe because they are trying to use the
same tablespace path. The PANIC is also expected as that's what we
want with data_sync_retry = off, which is the default, and the wanted
behavior to PANIC immediately and enforce WAL recovery should a fsync
fail. Obviously, not being able to have transparent tablespace
handling for multiple nodes on the same host is a problem, though this
implies grammar changes for CREATE TABLESPACE or having a sort of
node name handling which makes the experience trouble-less. Still
there is the argument that not all users would want both instances to
use the same tablespace path. So the problem is not as simple as it
looks, and the cost of solving it is not worth the use cases either.
--
Michael

At Tue, 23 Apr 2019 11:27:06 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190423022706.GG2712@paquier.xyz>

On Mon, Apr 22, 2019 at 03:52:59PM +0530, Kuntal Ghosh wrote:

I accept that configuring master-standby on the same machine for this
test is not okay. But, can we avoid the PANIC somehow? Or, is this
intentional and I should not include testtablespace in this case?

Well, it is a bit more than "not okay", as the primary and the
standby step on each other's toe because they are trying to use the
same tablespace path. The PANIC is also expected as that's what we
want with data_sync_retry = off, which is the default, and the wanted
behavior to PANIC immediately and enforce WAL recovery should a fsync
fail. Obviously, not being able to have transparent tablespace
handling for multiple nodes on the same host is a problem, though this
implies grammar changes for CREATE TABLESPACE or having a sort of
node name handling which makes the experience trouble-less. Still
there is the argument that not all users would want both instances to
use the same tablespace path. So the problem is not as simple as it
looks, and the cost of solving it is not worth the use cases either.

We could easily adopt a jail or chroot like feature to tablespace
paths. Suppose a new GUC(!), say, tablespace_chroot and the value
can contain replacements like %a, %p, %h, we would set the
variable as:

tablespace_chroot = '/somewhere/%p';

then the tablespace location is prefixed by '/somewhere/5432' for
the first server, '/somehwere/5433' for the second.

I think this is rahter a testing or debugging feature. This can
be apply to all paths, so the variable might be "path_prefix" or
something more generic than tablespace_chroot.

Does it make sense?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 23 Apr 2019 13:33:39 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190423.133339.113770648.horiguchi.kyotaro@lab.ntt.co.jp>

At Tue, 23 Apr 2019 11:27:06 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190423022706.GG2712@paquier.xyz>

On Mon, Apr 22, 2019 at 03:52:59PM +0530, Kuntal Ghosh wrote:

I accept that configuring master-standby on the same machine for this
test is not okay. But, can we avoid the PANIC somehow? Or, is this
intentional and I should not include testtablespace in this case?

Well, it is a bit more than "not okay", as the primary and the
standby step on each other's toe because they are trying to use the
same tablespace path. The PANIC is also expected as that's what we
want with data_sync_retry = off, which is the default, and the wanted
behavior to PANIC immediately and enforce WAL recovery should a fsync
fail. Obviously, not being able to have transparent tablespace
handling for multiple nodes on the same host is a problem, though this
implies grammar changes for CREATE TABLESPACE or having a sort of
node name handling which makes the experience trouble-less. Still
there is the argument that not all users would want both instances to
use the same tablespace path. So the problem is not as simple as it
looks, and the cost of solving it is not worth the use cases either.

We could easily adopt a jail or chroot like feature to tablespace
paths. Suppose a new GUC(!), say, tablespace_chroot and the value
can contain replacements like %a, %p, %h, we would set the
variable as:

tablespace_chroot = '/somewhere/%p';

then the tablespace location is prefixed by '/somewhere/5432' for
the first server, '/somehwere/5433' for the second.

I think this is rahter a testing or debugging feature. This can
be apply to all paths, so the variable might be "path_prefix" or

all paths out of $PGDATA directory. tablespace locations,
log_directory and stats_temp_directory?

something more generic than tablespace_chroot.

Does it make sense?

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Apr 23, 2019 at 01:33:39PM +0900, Kyotaro HORIGUCHI wrote:

I think this is rahter a testing or debugging feature. This can
be apply to all paths, so the variable might be "path_prefix" or
something more generic than tablespace_chroot.

Does it make sense?

A GUC which enforces object creation does not sound like a good idea
to me, and what you propose would still bite back, for example two
local nodes could use the same port, but a different Unix socket
path.
--
Michael

Hi,

On 2019-04-22 15:52:59 +0530, Kuntal Ghosh wrote:

Hello hackers,

With a master-standby setup configured on the same machine, I'm
getting a panic in tablespace test while running make installcheck.

1. CREATE TABLESPACE regress_tblspacewith LOCATION 'blah';
2. DROP TABLESPACE regress_tblspacewith;
3. CREATE TABLESPACE regress_tblspace LOCATION 'blah';
-- do some operations in this tablespace
4. DROP TABLESPACE regress_tblspace;

The master panics at the last statement when standby has completed
applying the WAL up to step 2 but hasn't started step 3.
PANIC: could not fsync file
"pg_tblspc/16387/PG_12_201904072/16384/16446": No such file or
directory

The reason is both the tablespace points to the same location. When
master tries to delete the new tablespace (and requests a checkpoint),
the corresponding folder is already deleted by the standby while
applying WAL to delete the old tablespace. I'm able to reproduce the
issue with the attached script.

sh standby-server-setup.sh
make installcheck

I accept that configuring master-standby on the same machine for this
test is not okay. But, can we avoid the PANIC somehow? Or, is this
intentional and I should not include testtablespace in this case?

FWIW, I think the right fix for this is to simply drop the requirement
that tablespace paths need to be absolute. It's not buying us anything,
it's just making things more complicated. We should just do a simple
check against the tablespace being inside PGDATA, and leave it at
that. Yes, that can be tricked, but so can the current system.

That'd make both regression tests easier, as well as operations.

Greetings,

Andres Freund

On Mon, Apr 22, 2019 at 11:00:03PM -0700, Andres Freund wrote:

FWIW, I think the right fix for this is to simply drop the requirement
that tablespace paths need to be absolute. It's not buying us anything,
it's just making things more complicated. We should just do a simple
check against the tablespace being inside PGDATA, and leave it at
that. Yes, that can be tricked, but so can the current system.

convert_and_check_filename() checks after that already, mostly. For
TAP tests I am not sure that this would help much though as all the
nodes of a given test use the same root path for their data folders,
so you cannot just use "../hoge/" as location. We already generate a
warning when a tablespace is in a data folder, as this causes issues
with recursion lookups of base backups. What do you mean in this
case? Forbidding the behavior?
--
Michael

At Tue, 23 Apr 2019 14:53:28 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190423055328.GK2712@paquier.xyz>

On Tue, Apr 23, 2019 at 01:33:39PM +0900, Kyotaro HORIGUCHI wrote:

I think this is rahter a testing or debugging feature. This can
be apply to all paths, so the variable might be "path_prefix" or
something more generic than tablespace_chroot.

Does it make sense?

A GUC which enforces object creation does not sound like a good idea
to me, and what you propose would still bite back, for example two
local nodes could use the same port, but a different Unix socket
path.

It's not necessarily be port number, but I agree that it's not a
good idea. I prefer allowing relative paths for tablespaces.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 23 Apr 2019 16:08:18 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190423070818.GM2712@paquier.xyz>

On Mon, Apr 22, 2019 at 11:00:03PM -0700, Andres Freund wrote:

FWIW, I think the right fix for this is to simply drop the requirement
that tablespace paths need to be absolute. It's not buying us anything,
it's just making things more complicated. We should just do a simple
check against the tablespace being inside PGDATA, and leave it at
that. Yes, that can be tricked, but so can the current system.

convert_and_check_filename() checks after that already, mostly. For
TAP tests I am not sure that this would help much though as all the
nodes of a given test use the same root path for their data folders,
so you cannot just use "../hoge/" as location. We already generate a
warning when a tablespace is in a data folder, as this causes issues
with recursion lookups of base backups. What do you mean in this
case? Forbidding the behavior?

Isn't it good enough just warning when we see pg_tblspc twice
while scanning? The check is not perfect for an "abosolute path"
that continas '/./' above pgdata directory.

For TAP tests, we can point generated temporary directory by
"../../<tmpdirsname>".

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 23 Apr 2019 17:44:18 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190423.174418.262292011.horiguchi.kyotaro@lab.ntt.co.jp>

At Tue, 23 Apr 2019 16:08:18 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190423070818.GM2712@paquier.xyz>

On Mon, Apr 22, 2019 at 11:00:03PM -0700, Andres Freund wrote:

FWIW, I think the right fix for this is to simply drop the requirement
that tablespace paths need to be absolute. It's not buying us anything,
it's just making things more complicated. We should just do a simple
check against the tablespace being inside PGDATA, and leave it at
that. Yes, that can be tricked, but so can the current system.

convert_and_check_filename() checks after that already, mostly. For
TAP tests I am not sure that this would help much though as all the
nodes of a given test use the same root path for their data folders,
so you cannot just use "../hoge/" as location. We already generate a
warning when a tablespace is in a data folder, as this causes issues
with recursion lookups of base backups. What do you mean in this
case? Forbidding the behavior?

Isn't it good enough just warning when we see pg_tblspc twice
while scanning? The check is not perfect for an "abosolute path"
that continas '/./' above pgdata directory.

I don't get basebackup recurse. How can I do that? basebackup
rejects non-empty direcoty as a tablespace directory. I'm not
sure about pg_upgrade but it's not a problem as far as we keep
waning on that kind of tablespace directory.

So I propose this:

- Allow relative path as a tablespace direcotry in exchange for
issueing WARNING.

=# CREATE TABLESPACE ts1 LOCATION '../../hoge';
"WARNING: tablespace location should be in absolute path"

- For abosolute paths, we keep warning as before. Of course we
don't bother '.' and '..'.

=# CREATE TABLESPACE ts1 LOCATION '/home/.../data';
"WARNING: tablespace location should not be in the data directory"

For TAP tests, we can point generated temporary directory by
"../../<tmpdirsname>".

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 23 Apr 2019 19:00:54 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190423.190054.262966274.horiguchi.kyotaro@lab.ntt.co.jp>

For TAP tests, we can point generated temporary directory by
"../../<tmpdirsname>".

Wrong. A generating tmpdir (how?) in "../" (that is, in the node
direcotry) would work.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

On 2019-04-23 16:08:18 +0900, Michael Paquier wrote:

On Mon, Apr 22, 2019 at 11:00:03PM -0700, Andres Freund wrote:

FWIW, I think the right fix for this is to simply drop the requirement
that tablespace paths need to be absolute. It's not buying us anything,
it's just making things more complicated. We should just do a simple
check against the tablespace being inside PGDATA, and leave it at
that. Yes, that can be tricked, but so can the current system.

convert_and_check_filename() checks after that already, mostly. For
TAP tests I am not sure that this would help much though as all the
nodes of a given test use the same root path for their data folders,
so you cannot just use "../hoge/" as location.

I don't see the problem here. Putting the primary and standby PGDATAs
into a subdirectory that also can contain a relatively referenced
tablespace seems trivial?

I'm not We already generate a warning when a tablespace is in a data
folder, as this causes issues with recursion lookups of base backups.
What do you mean in this case? Forbidding the behavior? -- Michael

I mostly am talking about replacing

Oid
CreateTableSpace(CreateTableSpaceStmt *stmt)
{
...
/*
* 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")));

with a check that forces relative paths to be outside of PGDATA (baring
symlinks). As far as I can tell convert_and_check_filename() would check
just about the opposite.

Greetings,

Andres Freund

At Tue, 23 Apr 2019 10:05:03 -0700, Andres Freund <andres@anarazel.de> wrote in <20190423170503.uw5jxrujqlozg23l@alap3.anarazel.de>

Hi,

On 2019-04-23 16:08:18 +0900, Michael Paquier wrote:

On Mon, Apr 22, 2019 at 11:00:03PM -0700, Andres Freund wrote:

FWIW, I think the right fix for this is to simply drop the requirement
that tablespace paths need to be absolute. It's not buying us anything,
it's just making things more complicated. We should just do a simple
check against the tablespace being inside PGDATA, and leave it at
that. Yes, that can be tricked, but so can the current system.

convert_and_check_filename() checks after that already, mostly. For
TAP tests I am not sure that this would help much though as all the
nodes of a given test use the same root path for their data folders,
so you cannot just use "../hoge/" as location.

I don't see the problem here. Putting the primary and standby PGDATAs
into a subdirectory that also can contain a relatively referenced
tablespace seems trivial?

I'm not We already generate a warning when a tablespace is in a data
folder, as this causes issues with recursion lookups of base backups.
What do you mean in this case? Forbidding the behavior? -- Michael

I mostly am talking about replacing

Oid
CreateTableSpace(CreateTableSpaceStmt *stmt)
{
...
/*
* 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")));

with a check that forces relative paths to be outside of PGDATA (baring
symlinks). As far as I can tell convert_and_check_filename() would check
just about the opposite.

We need to adjust relative path between PGDATA-based and
pg_tblspc based. The attached first patch does that.

- I'm not sure it is OK to use getcwd this way.

The second attached is TAP change to support tablespaces using
relative tablespaces. One issue here is is_in_data_directory
canonicalizes DataDir on-the-fly. It is needed when DataDir
contains '/./' or such. I think the canonicalization should be
done far earlier.

- This is tentative or sample. I'll visit the current discussion thread.

The third is test for this issue.

- Tablespace handling gets easier.

The fourth is the fix for the issue here.

- Not all possible simliar issue is not checked.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Sorry, I was in haste.

At Wed, 24 Apr 2019 13:18:45 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190424.131845.116224815.horiguchi.kyotaro@lab.ntt.co.jp>

At Tue, 23 Apr 2019 10:05:03 -0700, Andres Freund <andres@anarazel.de> wrote in <20190423170503.uw5jxrujqlozg23l@alap3.anarazel.de>

Hi,

On 2019-04-23 16:08:18 +0900, Michael Paquier wrote:

On Mon, Apr 22, 2019 at 11:00:03PM -0700, Andres Freund wrote:

FWIW, I think the right fix for this is to simply drop the requirement
that tablespace paths need to be absolute. It's not buying us anything,
it's just making things more complicated. We should just do a simple
check against the tablespace being inside PGDATA, and leave it at
that. Yes, that can be tricked, but so can the current system.

convert_and_check_filename() checks after that already, mostly. For
TAP tests I am not sure that this would help much though as all the
nodes of a given test use the same root path for their data folders,
so you cannot just use "../hoge/" as location.

I don't see the problem here. Putting the primary and standby PGDATAs
into a subdirectory that also can contain a relatively referenced
tablespace seems trivial?

I'm not We already generate a warning when a tablespace is in a data
folder, as this causes issues with recursion lookups of base backups.
What do you mean in this case? Forbidding the behavior? -- Michael

I mostly am talking about replacing

Oid
CreateTableSpace(CreateTableSpaceStmt *stmt)
{
...
/*
* 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")));

with a check that forces relative paths to be outside of PGDATA (baring
symlinks). As far as I can tell convert_and_check_filename() would check
just about the opposite.

We need to adjust relative path between PGDATA-based and
pg_tblspc based. The attached first patch does that.

- I'm not sure it is OK to use getcwd this way. Another issue
here is is_in_data_directory canonicalizes DataDir
on-the-fly. It is needed when DataDir contains '/./' or such. I
think the canonicalization should be done far earlier.

The second attached is TAP change to support tablespaces using
relative tablespaces.

- This is tentative or sample. I'll visit the current discussion thread.

The third is test for this issue.

- Tablespace handling gets easier.

The fourth is the fix for the issue here.

- Not all possible simliar issue is not checked.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

At Wed, 24 Apr 2019 13:23:04 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190424.132304.40676137.horiguchi.kyotaro@lab.ntt.co.jp>

with a check that forces relative paths to be outside of PGDATA (baring
symlinks). As far as I can tell convert_and_check_filename() would check
just about the opposite.

We need to adjust relative path between PGDATA-based and
pg_tblspc based. The attached first patch does that.

- I'm not sure it is OK to use getcwd this way. Another issue
here is is_in_data_directory canonicalizes DataDir
on-the-fly. It is needed when DataDir contains '/./' or such. I
think the canonicalization should be done far earlier.

The second attached is TAP change to support tablespaces using
relative tablespaces.

- This is tentative or sample. I'll visit the current discussion thread.

The third is test for this issue.

- Tablespace handling gets easier.

The fourth is the fix for the issue here.

- Not all possible simliar issue is not checked.

This is new version. Adjusted pg_basebackup's behavior to allow
relative mappings. But..

This is apparently out of a bug fix. What should I do with it?

Should we applying only 0004 (after further checking) or
something as bug fix, then register the rest for v13?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:

At Wed, 24 Apr 2019 13:23:04 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190424.132304.40676137.horiguchi.kyotaro@lab.ntt.co.jp>

We need to adjust relative path between PGDATA-based and
pg_tblspc based. The attached first patch does that.

This is new version. Adjusted pg_basebackup's behavior to allow
relative mappings. But..

I can't say that I like 0001 at all.  It adds a bunch of complication and
new failure modes (e.g., having to panic on chdir failure) in order to do
what exactly?  I've not been following the thread closely, but the
original problem is surely just a dont-do-that misconfiguration.  I also
suspect that this is assuming way too much about the semantics of getcwd
--- some filesystem configurations may have funny situations like multiple
paths to the same place.

0004 also seems like it's adding at least as many failure modes as
it removes. Moreover, isn't it just postponing the failure a little?
Later WAL might well try to touch the directory you skipped creation of.
We can't realistically decide that all WAL-application errors are
ignorable, but that seems like the direction this would have us go in.

regards, tom lane

Hi,

On 2019-04-24 10:13:09 -0400, Tom Lane wrote:

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:

At Wed, 24 Apr 2019 13:23:04 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190424.132304.40676137.horiguchi.kyotaro@lab.ntt.co.jp>

We need to adjust relative path between PGDATA-based and
pg_tblspc based. The attached first patch does that.

This is new version. Adjusted pg_basebackup's behavior to allow
relative mappings. But..

I can't say that I like 0001 at all.  It adds a bunch of complication and
new failure modes (e.g., having to panic on chdir failure) in order to do
what exactly?  I've not been following the thread closely, but the
original problem is surely just a dont-do-that misconfiguration.  I also
suspect that this is assuming way too much about the semantics of getcwd
--- some filesystem configurations may have funny situations like multiple
paths to the same place.

I'm not at all defending the conrete patch. But I think allowing
relative paths to tablespaces would solve a whole lot of practical
problems, while not meaningfully increasing failure modes. The inability
to reasonably test master/standby setups on a single machine is pretty
jarring (yes, one can use basebackup tablespace maps - but that doesn't
work well for new tablespaces). And for a lot of production setups
absolute paths suck too - it's far from a given that primary / standby
databases need to have the same exact path layout.

Greetings,

Andres Freund

Hi,

On 2019-04-24 17:02:28 +0900, Kyotaro HORIGUCHI wrote:

+/*
+ * Check if the path is in the data directory strictly.
+ */
+static bool
+is_in_data_directory(const char *path)
+{
+	char cwd[MAXPGPATH];
+	char abspath[MAXPGPATH];
+	char absdatadir[MAXPGPATH];
+
+	getcwd(cwd, MAXPGPATH);
+	if (chdir(path) < 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid directory \"%s\": %m", path)));
+
+	/* getcwd is defined as returning absolute path */
+	getcwd(abspath, MAXPGPATH);
+
+	/* DataDir needs to be canonicalized */
+	if (chdir(DataDir))
+		ereport(FATAL,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("could not chdir to the data directory \"%s\": %m",
+						DataDir)));
+	getcwd(absdatadir, MAXPGPATH);
+
+	/* this must succeed */
+	if (chdir(cwd))
+		ereport(FATAL,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("could not chdir to the current working directory \"%s\": %m",
+					 cwd)));
+
+	return path_is_prefix_of_path(absdatadir, abspath);
+}

This seems like a bad idea to me. Why don't we just use
make_absolute_path() on the proposed tablespace path, and then check
path_is_prefix_of() or such? Sure, that can be tricked using symlinks
etc, but that's already the case.

Greetings,

Andres Freund