pg_tablespace_location() failure with allow_in_place_tablespaces

At Fri, 4 Mar 2022 15:44:22 +0900, Michael Paquier <michael@paquier.xyz> wrote in

Hi all,

While playing with tablespaces and recovery in a TAP test, I have
noticed that retrieving the location of a tablespace created with
allow_in_place_tablespaces enabled fails in pg_tablespace_location(),
because readlink() sees a directory in this case.

ERROR: could not read symbolic link "pg_tblspc/16407": Invalid argument

The use may be limited to any automated testing and
allow_in_place_tablespaces is a developer GUC, still it seems to me
that there is an argument to allow the case rather than tweak any
tests to hardcode a path with the tablespace OID. And any other code
paths are able to handle such tablespaces, be they in recovery or in
tablespace create/drop.

+1

A junction point is a directory on WIN32 as far as I recall, but
pgreadlink() is here to ensure that we get the correct path on
a source found as pgwin32_is_junction(), so we can rely on that. This
stuff has led me to the attached.

Thoughts?

The function I think is expected to return a absolute path but it
returns a relative path for in-place tablespaces. While it is
apparently incovenient for general use, there might be a case where we
want to know whether the tablespace is in-place or not. So I'm not
sure which is better..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 04 Mar 2022 16:41:03 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Fri, 4 Mar 2022 15:44:22 +0900, Michael Paquier <michael@paquier.xyz> wrote in

The use may be limited to any automated testing and
allow_in_place_tablespaces is a developer GUC, still it seems to me
that there is an argument to allow the case rather than tweak any
tests to hardcode a path with the tablespace OID. And any other code
paths are able to handle such tablespaces, be they in recovery or in
tablespace create/drop.

+1

By the way, regardless of the patch, I got an error from pg_basebackup
for an in-place tablespace. pg_do_start_backup calls readlink
believing pg_tblspc/* is always a symlink.

# Running: pg_basebackup -D /home/horiguti/work/worktrees/tsp_replay_2/src/test/recovery/tmp_check/t_029_replay_tsp_drops_primary1_data/backup/my_backup -h /tmp/X8E4nbF4en -p 51584 --checkpoint fast --no-sync
WARNING: could not read symbolic link "pg_tblspc/16384": Invalid argument

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 04 Mar 2022 16:54:49 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Fri, 04 Mar 2022 16:41:03 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Fri, 4 Mar 2022 15:44:22 +0900, Michael Paquier <michael@paquier.xyz> wrote in

The use may be limited to any automated testing and
allow_in_place_tablespaces is a developer GUC, still it seems to me
that there is an argument to allow the case rather than tweak any
tests to hardcode a path with the tablespace OID. And any other code
paths are able to handle such tablespaces, be they in recovery or in
tablespace create/drop.

+1

By the way, regardless of the patch, I got an error from pg_basebackup
for an in-place tablespace. pg_do_start_backup calls readlink
believing pg_tblspc/* is always a symlink.

# Running: pg_basebackup -D /home/horiguti/work/worktrees/tsp_replay_2/src/test/recovery/tmp_check/t_029_replay_tsp_drops_primary1_data/backup/my_backup -h /tmp/X8E4nbF4en -p 51584 --checkpoint fast --no-sync
WARNING: could not read symbolic link "pg_tblspc/16384": Invalid argument

So now we know that there are three places that needs the same
processing.

pg_tablespace_location: this patch tries to fix
sendDir: it already supports in-place tsp
do_pg_start_backup: not supports in-place tsp yet.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 04 Mar 2022 17:28:45 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

By the way, regardless of the patch, I got an error from pg_basebackup
for an in-place tablespace. pg_do_start_backup calls readlink
believing pg_tblspc/* is always a symlink.

# Running: pg_basebackup -D /home/horiguti/work/worktrees/tsp_replay_2/src/test/recovery/tmp_check/t_029_replay_tsp_drops_primary1_data/backup/my_backup -h /tmp/X8E4nbF4en -p 51584 --checkpoint fast --no-sync
WARNING: could not read symbolic link "pg_tblspc/16384": Invalid argument

So now we know that there are three places that needs the same
processing.

pg_tablespace_location: this patch tries to fix
sendDir: it already supports in-place tsp
do_pg_start_backup: not supports in-place tsp yet.

And I made a quick hack on do_pg_start_backup. And I found that
pg_basebackup copies in-place tablespaces under the *current
directory*, which is not ok at all:(

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Mar 4, 2022 at 10:04 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

And I made a quick hack on do_pg_start_backup. And I found that
pg_basebackup copies in-place tablespaces under the *current
directory*, which is not ok at all:(

Hmm. Which OS are you on? Looks OK here -- the "in place" tablespace
gets copied as a directory under pg_tblspc, no symlink:

postgres=# set allow_in_place_tablespaces = on;
SET
postgres=# create tablespace ts1 location '';
CREATE TABLESPACE
postgres=# create table t (i int) tablespace ts1;
CREATE TABLE
postgres=# insert into t values (1), (2);
INSERT 0 2
postgres=# create user replication replication;
CREATE ROLE

$ pg_basebackup --user replication -D pgdata2
$ ls -slaph pgdata/pg_tblspc/
total 4.0K
0 drwx------ 3 tmunro tmunro 19 Mar 4 23:16 ./
4.0K drwx------ 19 tmunro tmunro 4.0K Mar 4 23:16 ../
0 drwx------ 3 tmunro tmunro 29 Mar 4 23:16 16384/
$ ls -slaph pgdata2/pg_tblspc/
total 4.0K
0 drwx------ 3 tmunro tmunro 19 Mar 4 23:16 ./
4.0K drwx------ 19 tmunro tmunro 4.0K Mar 4 23:16 ../
0 drwx------ 3 tmunro tmunro 29 Mar 4 23:16 16384/
$ ls -slaph pgdata/pg_tblspc/16384/PG_15_202203031/5/
total 8.0K
0 drwx------ 2 tmunro tmunro 19 Mar 4 23:16 ./
0 drwx------ 3 tmunro tmunro 15 Mar 4 23:16 ../
8.0K -rw------- 1 tmunro tmunro 8.0K Mar 4 23:16 16385
$ ls -slaph pgdata2/pg_tblspc/16384/PG_15_202203031/5/
total 8.0K
0 drwx------ 2 tmunro tmunro 19 Mar 4 23:16 ./
0 drwx------ 3 tmunro tmunro 15 Mar 4 23:16 ../
8.0K -rw------- 1 tmunro tmunro 8.0K Mar 4 23:16 16385

The warning from readlink() while making the mapping file isn't ideal,
and perhaps we should suppress that with something like the attached.
Or does the missing map file entry break something on Windows?

On Fri, Mar 04, 2022 at 06:04:00PM +0900, Kyotaro Horiguchi wrote:

And I made a quick hack on do_pg_start_backup. And I found that
pg_basebackup copies in-place tablespaces under the *current
directory*, which is not ok at all:(

Yeah, I have noticed that as well while testing such configurations a
couple of hours ago, but I am not sure yet how much we need to care
about that as in-place tablespaces are included in the main data
directory anyway, which would be fine for most test purposes we
usually care about. Perhaps this has an impact on the patch posted on
the thread that wants to improve the guarantees around tablespace
directory structures, but I have not studied this thread much to have
an opinion. And it is Friday.
--
Michael

At Fri, 4 Mar 2022 23:26:43 +1300, Thomas Munro <thomas.munro@gmail.com> wrote in

On Fri, Mar 4, 2022 at 10:04 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

And I made a quick hack on do_pg_start_backup. And I found that
pg_basebackup copies in-place tablespaces under the *current
directory*, which is not ok at all:(

Hmm. Which OS are you on? Looks OK here -- the "in place" tablespace
gets copied as a directory under pg_tblspc, no symlink:

The warning from readlink() while making the mapping file isn't ideal,
and perhaps we should suppress that with something like the attached.
Or does the missing map file entry break something on Windows?

Ah.. Ok, somehow I thought that pg_basebackup failed for readlink
failure and the tweak I made made things worse. I got to make it
work.

Thanks!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Mar 04, 2022 at 03:44:22PM +0900, Michael Paquier wrote:

The use may be limited to any automated testing and
allow_in_place_tablespaces is a developer GUC, still it seems to me
that there is an argument to allow the case rather than tweak any
tests to hardcode a path with the tablespace OID. And any other code
paths are able to handle such tablespaces, be they in recovery or in
tablespace create/drop.

A junction point is a directory on WIN32 as far as I recall, but
pgreadlink() is here to ensure that we get the correct path on
a source found as pgwin32_is_junction(), so we can rely on that. This
stuff has led me to the attached.

Thomas, I'd rather fix this for the sake of the tests. One point is
that the function returns a relative path for in-place tablespaces,
but it would be easy enough to append a DataDir. What do you think?
--
Michael

On Fri, Mar 04, 2022 at 11:26:43PM +1300, Thomas Munro wrote:

The warning from readlink() while making the mapping file isn't ideal,
and perhaps we should suppress that with something like the attached.
Or does the missing map file entry break something on Windows?

@@ -8292,6 +8293,10 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,

snprintf(fullpath, sizeof(fullpath), "pg_tblspc/%s", de->d_name);

+	/* Skip in-place tablespaces (testing use only) */
+	if (get_dirent_type(fullpath, de, false, ERROR) == PGFILETYPE_DIR)
+		continue;

I saw the warning when testing base backups with in-place tablespaces
and it did not annoy me much, but, yes, that can be confusing.

Junction points are directories, no? Are you sure that this works
correctly on WIN32? It seems to me that we'd better use readlink()
only for entries in pg_tlbspc/ that are PGFILETYPE_LNK on non-WIN32
and pgwin32_is_junction() on WIN32.
--
Michael

On Tue, Mar 8, 2022 at 12:58 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Mar 04, 2022 at 11:26:43PM +1300, Thomas Munro wrote:

+     /* Skip in-place tablespaces (testing use only) */
+     if (get_dirent_type(fullpath, de, false, ERROR) == PGFILETYPE_DIR)
+             continue;

I saw the warning when testing base backups with in-place tablespaces
and it did not annoy me much, but, yes, that can be confusing.

Junction points are directories, no? Are you sure that this works
correctly on WIN32? It seems to me that we'd better use readlink()
only for entries in pg_tlbspc/ that are PGFILETYPE_LNK on non-WIN32
and pgwin32_is_junction() on WIN32.

Thanks, you're right. Test on a Win10 VM. Here's a new version.

On Tue, Mar 8, 2022 at 10:39 AM Thomas Munro <thomas.munro@gmail.com> wrote:

Test on a Win10 VM.

Erm, "Tested" (as in, I tested), I meant to write...

At Tue, 8 Mar 2022 10:39:06 +1300, Thomas Munro <thomas.munro@gmail.com> wrote in

On Tue, Mar 8, 2022 at 12:58 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Mar 04, 2022 at 11:26:43PM +1300, Thomas Munro wrote:

+     /* Skip in-place tablespaces (testing use only) */
+     if (get_dirent_type(fullpath, de, false, ERROR) == PGFILETYPE_DIR)
+             continue;

I saw the warning when testing base backups with in-place tablespaces
and it did not annoy me much, but, yes, that can be confusing.

Junction points are directories, no? Are you sure that this works
correctly on WIN32? It seems to me that we'd better use readlink()
only for entries in pg_tlbspc/ that are PGFILETYPE_LNK on non-WIN32
and pgwin32_is_junction() on WIN32.

Thanks, you're right. Test on a Win10 VM. Here's a new version.

Thanks! It works for me on CentOS8 and Windows11.

FYI, on Windows11, pg_basebackup didn't work correctly without the
patch. So this looks like fixing an undiscovered bug as well.

===

pg_basebackup -D copy

WARNING: could not read symbolic link "pg_tblspc/16384": Invalid argument
pg_basebackup: error: tar member has empty name

dir copy

Volume in drive C has no label.
Volume serial number: 10C6-4BA6

Directory of c:\..\copy

2022/03/08 09:53 <DIR> .
2022/03/08 09:53 <DIR> ..
2022/03/08 09:53 0 nbase.tar
2022/03/08 09:53 <DIR> pg_wal
1 File(s) 0 bytes
3 Dir(s) 171,920,613,376 bytes free

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Mar 08, 2022 at 10:06:50AM +0900, Kyotaro Horiguchi wrote:

At Tue, 8 Mar 2022 10:39:06 +1300, Thomas Munro <thomas.munro@gmail.com> wrote in

Thanks, you're right. Test on a Win10 VM. Here's a new version.

Looks fine to me.

FYI, on Windows11, pg_basebackup didn't work correctly without the
patch. So this looks like fixing an undiscovered bug as well.

Well, that's not really a long-time bug but just a side effect of
in-place tablespaces because we don't use them in many test cases
yet, is it?

pg_basebackup -D copy

WARNING: could not read symbolic link "pg_tblspc/16384": Invalid argument
pg_basebackup: error: tar member has empty name

1 File(s) 0 bytes
3 Dir(s) 171,920,613,376 bytes free

That's a lot of free space.
--
Michael

At Tue, 8 Mar 2022 10:28:46 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Tue, Mar 08, 2022 at 10:06:50AM +0900, Kyotaro Horiguchi wrote:

At Tue, 8 Mar 2022 10:39:06 +1300, Thomas Munro <thomas.munro@gmail.com> wrote in

Thanks, you're right. Test on a Win10 VM. Here's a new version.

Looks fine to me.

FYI, on Windows11, pg_basebackup didn't work correctly without the
patch. So this looks like fixing an undiscovered bug as well.

Well, that's not really a long-time bug but just a side effect of
in-place tablespaces because we don't use them in many test cases
yet, is it?

No, we don't. So just FYI.

pg_basebackup -D copy

WARNING: could not read symbolic link "pg_tblspc/16384": Invalid argument
pg_basebackup: error: tar member has empty name

1 File(s) 0 bytes
3 Dir(s) 171,920,613,376 bytes free

That's a lot of free space.

The laptop has a 512GB storage, so 160GB is pretty normal, maybe.
129GB of the storage is used by some VMs..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Mar 8, 2022 at 4:01 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Tue, 8 Mar 2022 10:28:46 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Tue, Mar 08, 2022 at 10:06:50AM +0900, Kyotaro Horiguchi wrote:

At Tue, 8 Mar 2022 10:39:06 +1300, Thomas Munro <thomas.munro@gmail.com> wrote in

Thanks, you're right. Test on a Win10 VM. Here's a new version.

Looks fine to me.

FYI, on Windows11, pg_basebackup didn't work correctly without the
patch. So this looks like fixing an undiscovered bug as well.

Well, that's not really a long-time bug but just a side effect of
in-place tablespaces because we don't use them in many test cases
yet, is it?

No, we don't. So just FYI.

Ok, I pushed the fix for pg_basebackup.

As for the complaint about pg_tablespace_location() failing, would it
be better to return an empty string? That's what was passed in as
LOCATION. Something like the attached.

On Tue, Mar 15, 2022 at 2:33 PM Thomas Munro <thomas.munro@gmail.com> wrote:

As for the complaint about pg_tablespace_location() failing, would it
be better to return an empty string? That's what was passed in as
LOCATION. Something like the attached.

(Hrrmm, the contract for pgwin32_is_junction() is a little weird:
false means "success, but no" and also "failure, you should check
errno". But we never do.)

On Tue, Mar 15, 2022 at 02:33:17PM +1300, Thomas Munro wrote:

Ok, I pushed the fix for pg_basebackup.

As for the complaint about pg_tablespace_location() failing, would it
be better to return an empty string? That's what was passed in as
LOCATION. Something like the attached.

Hmm, I don't think so. The point of the function is to be able to
know the location of a tablespace at SQL level so as we don't have any
need to hardcode its location within any external tests (be it a
pg_regress test or a TAP test) based on how in-place tablespace paths
are built in the backend, so I think that we'd better report either a
relative path from data_directory or an absolute path, but not an
empty string.

In any case, I'd suggest to add a regression test. What I have sent
upthread would be portable enough.
--
Michael

On Tue, Mar 15, 2022 at 2:50 PM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Mar 15, 2022 at 02:33:17PM +1300, Thomas Munro wrote:

As for the complaint about pg_tablespace_location() failing, would it
be better to return an empty string? That's what was passed in as
LOCATION. Something like the attached.

Hmm, I don't think so. The point of the function is to be able to
know the location of a tablespace at SQL level so as we don't have any
need to hardcode its location within any external tests (be it a
pg_regress test or a TAP test) based on how in-place tablespace paths
are built in the backend, so I think that we'd better report either a
relative path from data_directory or an absolute path, but not an
empty string.

In any case, I'd suggest to add a regression test. What I have sent
upthread would be portable enough.

Fair enough. No objections here.

On Tue, Mar 15, 2022 at 03:55:56PM +1300, Thomas Munro wrote:

On Tue, Mar 15, 2022 at 2:50 PM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Mar 15, 2022 at 02:33:17PM +1300, Thomas Munro wrote:

As for the complaint about pg_tablespace_location() failing, would it
be better to return an empty string? That's what was passed in as
LOCATION. Something like the attached.

Hmm, I don't think so. The point of the function is to be able to
know the location of a tablespace at SQL level so as we don't have any
need to hardcode its location within any external tests (be it a
pg_regress test or a TAP test) based on how in-place tablespace paths
are built in the backend, so I think that we'd better report either a
relative path from data_directory or an absolute path, but not an
empty string.

In any case, I'd suggest to add a regression test. What I have sent
upthread would be portable enough.

Fair enough. No objections here.

So, which one of a relative path or an absolute path do you think
would be better for the user? My preference tends toward the relative
path, as we know that all those tablespaces stay in pg_tblspc/ so one
can make the difference with normal tablespaces more easily. The
barrier is thin, though :p
--
Michael