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);
signature.asc
Description: PGP signature
