Re: [PATCH] Add pg_get_tablespace_ddl() function to reconstruct CREATE TABLESPACE statement

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)