Subject: pg_tablespace_location() failure with allow_in_place_tablespaces

From: Michael Paquier <[email-masked]>
Date: Fri, 4 Mar 2022 15:44:22 +0900

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
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"
 
 #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 = PG_GETARG_OID(0);
 	char		sourcepath[MAXPGPATH];
-	char		targetpath[MAXPGPATH];
-	int			rllen;
+	struct stat	st;
 
 	/*
 	 * 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);
 
-	rllen = 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 >= sizeof(targetpath))
-		ereport(ERROR,
-				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
-				 errmsg("symbolic link \"%s\" target is too long",
-						sourcepath)));
-	targetpath[rllen] = '\0';
+	}
+	else if (
+#ifndef WIN32
+		S_ISLNK(st.st_mode)
+#else
+		pgwin32_is_junction(pathbuf)
+#endif
+		)
+	{
+		char		targetpath[MAXPGPATH];
+		int			rllen;
 
-	PG_RETURN_TEXT_P(cstring_to_text(targetpath));
+		rllen = 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 >= sizeof(targetpath))
+			ereport(ERROR,
+					(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+					 errmsg("symbolic link \"%s\" target is too long",
+							sourcepath)));
+		targetpath[rllen] = '\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/expected/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 = '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_tablespaces,
+-- 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 = 'regress_tblspace';
+ regexp_replace 
+----------------
+ pg_tblspc/NNN
+(1 row)
+
 -- try setting and resetting some properties for the new tablespace
 ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1);
 ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true);  -- fail
diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tablespace.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;
 
 -- create a tablespace we can use
 CREATE TABLESPACE regress_tblspace LOCATION '';
+-- This returns a relative path as of an effect of allow_in_place_tablespaces,
+-- 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 = 'regress_tblspace';
 
 -- try setting and resetting some properties for the new tablespace
 ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1);

---

From: Michael Paquier <[email-masked]>
Date: Mon, 7 Mar 2022 20:36:20 +0900

> 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