Date: Fri, 4 Mar 2022 15:44:22 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: pg_tablespace_location() failure with allow_in_place_tablespaces
Message-ID: <YiG1RleON1WBcLnX@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="XOidSgjRBix8NQr9"
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/YiG1RleON1WBcLnX%40paquier.xyz>
Precedence: bulk


--XOidSgjRBix8NQr9
Content-Type: multipart/mixed; boundary="W2cpSz60Kc1pNB+3"
Content-Disposition: inline


--W2cpSz60Kc1pNB+3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

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.

Thoughts?
--
Michael

--W2cpSz60Kc1pNB+3
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="tbspace-inplace-location.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index e79eb6b478..59b8f8196c 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -15,6 +15,7 @@
 #include "postgres.h"
=20
 #include <sys/file.h>
+#include <sys/stat.h>
 #include <dirent.h>
 #include <fcntl.h>
 #include <math.h>
@@ -307,8 +308,7 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
 {
 	Oid			tablespaceOid =3D PG_GETARG_OID(0);
 	char		sourcepath[MAXPGPATH];
-	char		targetpath[MAXPGPATH];
-	int			rllen;
+	struct stat	st;
=20
 	/*
 	 * It's useful to apply this function to pg_class.reltablespace, wherein
@@ -333,20 +333,57 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
 	 */
 	snprintf(sourcepath, sizeof(sourcepath), "pg_tblspc/%u", tablespaceOid);
=20
-	rllen =3D readlink(sourcepath, targetpath, sizeof(targetpath));
-	if (rllen < 0)
+	/*
+	 * Before reading the link, check if it is a link or a directory.
+	 * A directory is possible for a tablespace created with
+	 * allow_in_place_tablespaces enabled.  On Windows, junction points
+	 * are directories, which is why this is checked first.
+	 */
+	if (lstat(sourcepath, &st) < 0)
+	{
 		ereport(ERROR,
 				(errcode_for_file_access(),
-				 errmsg("could not read symbolic link \"%s\": %m",
+				 errmsg("could not stat file \"%s\": %m",
 						sourcepath)));
-	if (rllen >=3D sizeof(targetpath))
-		ereport(ERROR,
-				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
-				 errmsg("symbolic link \"%s\" target is too long",
-						sourcepath)));
-	targetpath[rllen] =3D '\0';
+	}
+	else if (
+#ifndef WIN32
+		S_ISLNK(st.st_mode)
+#else
+		pgwin32_is_junction(pathbuf)
+#endif
+		)
+	{
+		char		targetpath[MAXPGPATH];
+		int			rllen;
=20
-	PG_RETURN_TEXT_P(cstring_to_text(targetpath));
+		rllen =3D readlink(sourcepath, targetpath, sizeof(targetpath));
+		if (rllen < 0)
+			ereport(ERROR,
+					(errcode_for_file_access(),
+					 errmsg("could not read symbolic link \"%s\": %m",
+							sourcepath)));
+		if (rllen >=3D sizeof(targetpath))
+			ereport(ERROR,
+					(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+					 errmsg("symbolic link \"%s\" target is too long",
+							sourcepath)));
+		targetpath[rllen] =3D '\0';
+
+		PG_RETURN_TEXT_P(cstring_to_text(targetpath));
+	}
+	else if (S_ISDIR(st.st_mode))
+	{
+		/*
+		 * For a directory, return the relative path of the source,
+		 * as created by allow_in_place_tablespaces.  This is useful
+		 * for regression tests.
+		 */
+		PG_RETURN_TEXT_P(cstring_to_text(sourcepath));
+	}
+	else
+		elog(ERROR, "\"%s\" is not a directory or symbolic link",
+			 sourcepath);
 #else
 	ereport(ERROR,
 			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/ex=
pected/tablespace.out
index 2dfbcfdebe..473fe8c28e 100644
--- a/src/test/regress/expected/tablespace.out
+++ b/src/test/regress/expected/tablespace.out
@@ -24,6 +24,15 @@ SELECT spcoptions FROM pg_tablespace WHERE spcname =3D '=
regress_tblspacewith';
 DROP TABLESPACE regress_tblspacewith;
 -- create a tablespace we can use
 CREATE TABLESPACE regress_tblspace LOCATION '';
+-- This returns a relative path as of an effect of allow_in_place_tablespa=
ces,
+-- masking the tablespace OID used in the path name.
+SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\=
1/NNN')
+  FROM pg_tablespace  WHERE spcname =3D 'regress_tblspace';
+ regexp_replace=20
+----------------
+ pg_tblspc/NNN
+(1 row)
+
 -- try setting and resetting some properties for the new tablespace
 ALTER TABLESPACE regress_tblspace SET (random_page_cost =3D 1.0, seq_page_=
cost =3D 1.1);
 ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter =3D true=
);  -- fail
diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tab=
lespace.sql
index 896f05cea3..0949a28488 100644
--- a/src/test/regress/sql/tablespace.sql
+++ b/src/test/regress/sql/tablespace.sql
@@ -22,6 +22,10 @@ DROP TABLESPACE regress_tblspacewith;
=20
 -- create a tablespace we can use
 CREATE TABLESPACE regress_tblspace LOCATION '';
+-- This returns a relative path as of an effect of allow_in_place_tablespa=
ces,
+-- masking the tablespace OID used in the path name.
+SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\=
1/NNN')
+  FROM pg_tablespace  WHERE spcname =3D 'regress_tblspace';
=20
 -- try setting and resetting some properties for the new tablespace
 ALTER TABLESPACE regress_tblspace SET (random_page_cost =3D 1.0, seq_page_=
cost =3D 1.1);

--W2cpSz60Kc1pNB+3--

--XOidSgjRBix8NQr9
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmIhtUYACgkQnvQgOdby
QH076BAAjcjaccKnfbbI0VhjSdS70QAMy0uhCrMsdB3hjd6efUyxMBAuucCvV5Ux
A5gyL/nyJKW7dhvH3P3CrH00x8JzpnaE9VEQzHbL59CoeVE2k5gaK2fCu4fsUdKH
+81ewr0UHQhSlGSG3DTpm95ywZor4ZS4odyU4yOfz9juesufeItr6QiADJBXBgXa
sF+Klb+5/gV/rYTDxb04pHacFA90vOFRmNAI6jBDl5TtCMYB1Hcvhc6fuZhImMAf
PWzKmrvtgIZmOXy47PBNCuteFDjIqkvo4nGgg0U2qnL37DonOGcMnr+nhLfemvNR
hcswmJJa+Gk0eCU617/qquXNNqKaOzmFoLyXHyolymxbvLR5EHnlXK0HF20YW3v+
AZRmnPqENV27+lfXovV57NcacCmWAJGGFWLh9DDBxwMLDpl76mUEh7/lofedSs8H
+4JVl9WGxp0FPdD8QrR1SWyDx7mWzRQ7wW+q3qaIXiYtISmmXTTUFeiUwRfy3E5D
sqzqePVJr6nFJ+hhtql7ybDEtNROZX0RrPnvQ831VruoVl7++aklMo/tm5b9UTB0
hZI6VlwAAv6QsLQsFD5R7O48F7tXnkpGGg+6ddOJwy5HnBbKXgf4PSGLO4ZsJi1K
1b4u+88o6jZkcIu/VIjIWk7YAiRG4BaOzoCzZr4Tj0eKgGUw4q4=
=EodV
-----END PGP SIGNATURE-----

--XOidSgjRBix8NQr9--