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