Thread

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

Manni Wood <manni.wood@enterprisedb.com> — 2025-11-12T15:26:20Z

On Tue, Nov 11, 2025 at 9:16 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

> 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)
>

Thanks, Álvaro, for your continued help with this.

I have attached v11 patches that use all of the fixes from your
review.patch.txt.

I have built and tested this using both make/autotools and meson, and I had
Nishant (thanks, Nishant!) look at these before posting, so hopefully
everything will build correctly.
-- 
-- Manni Wood EDB: https://www.enterprisedb.com