On 2025-Nov-10, Nishant Sharma wrote:
> PFA, v10 patch set.
I propose the following changes for 0001, in patch hunk ordering.
1. pg_tablespace_location was introduced in 2011 (commit 16d8e594acd9),
so claim copyright starting at that point.
2. readlink(2) claims, at least in my system, to need <unistd.h>. Add
that.
3. get_tablespace_loc_string() is such an ugly name. Why not
get_tablespace_location()?
3. The initialization of sourcepath and targetpath are mostly pointless
(see below), so I'd leave it out.
3a. (Also, it's not clear to me that initializing to "{ '\0' }" is a
great idea. I understand that the C standard says that an
initialization to {0} zeroes the whole struct, but if you try to pass
some other char value, it actually fills everything else with zeroes
rather than the other char value. So hiding the 0 byte as a \0 char is
misleading.)
3b. Also, if you had zeroed targetpath at initialization time, there
would no longer be a need to print a zero byte after calling readlink(),
so you could have removed the "targetpath[rllen] = '\0';" line.
However as I said above, I'm not a fan of unnecessary initialization.
4. Using StringInfo in this function is pointless. You use that when
you're going to do a bunch of string manipulation ops, appending more
data after the first, or using sprintf() formatted strings and so on.
But here you return just one or two possible strings with no
construction involved. Might as well use standard pstrdup() as needed,
which keeps the code simple.
5. Single-statement blocks need no braces.
6. ereport() used to require an extra set of parenthesis, but no more.
Remove those.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)
commit 08f168ab38d635f9e33480b506f3a0d667f80244
Author: Álvaro Herrera <[email protected]> [Álvaro Herrera
<[email protected]>]
AuthorDate: Tue Nov 11 15:48:45 2025 +0100
CommitDate: Tue Nov 11 15:48:45 2025 +0100
[]review changes
diff --git a/src/backend/catalog/pg_tablespace.c
b/src/backend/catalog/pg_tablespace.c
index c00888456c8..d23aa7ae274 100644
--- a/src/backend/catalog/pg_tablespace.c
+++ b/src/backend/catalog/pg_tablespace.c
@@ -3,7 +3,7 @@
* pg_tablespace.c
* routines to support tablespaces
*
- * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 2011-2025, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
*
@@ -14,6 +14,7 @@
*/
#include "postgres.h"
+#include <unistd.h>
#include <sys/stat.h>
#include "catalog/pg_tablespace.h"
@@ -22,25 +23,21 @@
/*
- * get_tablespace_loc_string - Get a tablespace's location as a C-string for a
- * tablespace from its OID.
+ * get_tablespace_location
+ * Get a tablespace's location as a C-string, by its OID
*/
char *
-get_tablespace_loc_string(Oid tablespaceOid)
+get_tablespace_location(Oid tablespaceOid)
{
- char sourcepath[MAXPGPATH] = {'\0'};
- char targetpath[MAXPGPATH] = {'\0'};
+ char sourcepath[MAXPGPATH];
+ char targetpath[MAXPGPATH];
int rllen;
struct stat st;
- StringInfoData buf;
-
- initStringInfo(&buf);
- appendStringInfoString(&buf, "");
/*
- * It's useful to apply this function to pg_class.reltablespace, wherein
- * zero means "the database's default tablespace". So, rather than
- * throwing an error for zero, we choose to assume that's what is meant.
+ * It's useful to apply this to pg_class.reltablespace, wherein zero
means
+ * "the database's default tablespace". So, rather than throwing an
error
+ * for zero, we choose to assume that's what is meant.
*/
if (tablespaceOid == InvalidOid)
tablespaceOid = MyDatabaseTableSpace;
@@ -50,7 +47,7 @@ get_tablespace_loc_string(Oid tablespaceOid)
*/
if (tablespaceOid == DEFAULTTABLESPACE_OID ||
tablespaceOid == GLOBALTABLESPACE_OID)
- return buf.data;
+ return pstrdup("");
/*
* Find the location of the tablespace by reading the symbolic link that
@@ -65,36 +62,29 @@ get_tablespace_loc_string(Oid tablespaceOid)
* found, a relative path to the data directory is returned.
*/
if (lstat(sourcepath, &st) < 0)
- {
ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m",
- sourcepath)));
- }
+ errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m",
+ sourcepath));
if (!S_ISLNK(st.st_mode))
- {
- appendStringInfoString(&buf, sourcepath);
- return buf.data;
- }
+ return pstrdup(sourcepath);
/*
- * In presence of a link or a junction point, return the path pointing
to.
+ * In presence of a link or a junction point, return the path pointed
to.
*/
rllen = readlink(sourcepath, targetpath, sizeof(targetpath));
if (rllen < 0)
ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read symbolic link \"%s\":
%m",
- sourcepath)));
+ 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)));
+ errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("symbolic link \"%s\" target is too
long",
+ sourcepath));
targetpath[rllen] = '\0';
- appendStringInfoString(&buf, targetpath);
-
- return buf.data;
+ return pstrdup(targetpath);
}
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index cb99d7435eb..a365c432d34 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -318,7 +318,7 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
char *tablespaceLoc;
/* Get LOCATION string from its OID */
- tablespaceLoc = get_tablespace_loc_string(tablespaceOid);
+ tablespaceLoc = get_tablespace_location(tablespaceOid);
PG_RETURN_TEXT_P(cstring_to_text(tablespaceLoc));
}
diff --git a/src/include/catalog/pg_tablespace.h
b/src/include/catalog/pg_tablespace.h
index f065cff9ddc..7816d779d8c 100644
--- a/src/include/catalog/pg_tablespace.h
+++ b/src/include/catalog/pg_tablespace.h
@@ -54,6 +54,6 @@ DECLARE_UNIQUE_INDEX(pg_tablespace_spcname_index, 2698,
TablespaceNameIndexId, p
MAKE_SYSCACHE(TABLESPACEOID, pg_tablespace_oid_index, 4);
-extern char *get_tablespace_loc_string(Oid tablespaceOid);
+extern char *get_tablespace_location(Oid tablespaceOid);
#endif /* PG_TABLESPACE_H */
