Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add infrastructure for pg_get_*_ddl functions

  2. Add pg_get_tablespace_ddl() function

  3. Split out innards of pg_tablespace_location()

  4. Remove spclocation field from pg_tablespace

  1. [PATCH] Add pg_get_tablespace_ddl() function to reconstruct CREATE TABLESPACE statement

    Manni Wood <manni.wood@enterprisedb.com> — 2025-10-29T01:23:50Z

    Hello!

I am submitting a patch as a part of a larger Retail DDL functions project
described by Andrew Dunstan here:
https://www.postgresql.org/message-id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net

This patch creates a function pg_get_tablespace_ddl, designed to retrieve
the full DDL statement for a tablespace. Users can obtain the DDL by
providing the tablespace name, like so:

    SELECT pg_get_tablespace_ddl('regress_owner_tblsp');
                                           pg_get_tablespace_ddl

---------------------------------------------------------------------------------------------------
     CREATE TABLESPACE regress_owner_tblsp OWNER regress_user LOCATION ''
WITH (random_page_cost = 3);


This patch includes documentation, comments, and regression tests, all of
which pass successfully.

--
Best,

Manni Wood
EnterpriseDB

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

    Jim Jones <jim.jones@uni-muenster.de> — 2025-10-31T15:36:24Z

    Hi Manni,

Thanks for the patch!

On 29/10/2025 02:23, Manni Wood wrote:
> This patch creates a function pg_get_tablespace_ddl, designed to
> retrieve the full DDL statement for a tablespace. Users can obtain the
> DDL by providing the tablespace name, like so:
> 
>     SELECT pg_get_tablespace_ddl('regress_owner_tblsp');
>                                            pg_get_tablespace_ddl
>    
> ---------------------------------------------------------------------------------------------------
>      CREATE TABLESPACE regress_owner_tblsp OWNER regress_user LOCATION
> '' WITH (random_page_cost = 3);


Here my first comments regarding usability:

== quoted identifier ==

Tablespace names containing quoted identifiers cannot be parsed:

postgres=# CREATE TABLESPACE "My TS" LOCATION '/tmp/ts';
CREATE TABLESPACE
postgres=# SELECT pg_get_tablespace_ddl('"My TS"');
ERROR:  tablespace ""My TS"" does not exist

The following works, but I guess it shouldn't:

postgres=# SELECT pg_get_tablespace_ddl('My TS');
             pg_get_tablespace_ddl
-----------------------------------------------
 CREATE TABLESPACE "My TS" LOCATION '/tmp/ts';
(1 row)

The same applies for unicode characters:

postgres=# CREATE TABLESPACE "🐘" LOCATION '/tmp/ts';
CREATE TABLESPACE
postgres=# SELECT pg_get_tablespace_ddl('"🐘"');
ERROR:  tablespace ""🐘"" does not exist
postgres=# SELECT pg_get_tablespace_ddl('🐘');
           pg_get_tablespace_ddl
--------------------------------------------
 CREATE TABLESPACE "🐘" LOCATION '/tmp/ts';
(1 row)


== option precision ==

There is a precision loss in the options:

postgres=# CREATE TABLESPACE ts OWNER u1 LOCATION '/tmp/ts' WITH
(seq_page_cost = 1.12345678910, random_page_cost = 1.12345678910,
effective_io_concurrency = 17, maintenance_io_concurrency = 18);
CREATE TABLESPACE
postgres=# SELECT pg_get_tablespace_ddl('ts');

   pg_get_tablespace_ddl

---------------------------------------------------------------------------------------------------------------------------------------------
---------------------------------
 CREATE TABLESPACE ts OWNER u1 LOCATION '/tmp/ts' WITH (random_page_cost
= 1.12346, seq_page_cost = 1.12346, effective_io_concurrency = 17, m
aintenance_io_concurrency = 18);
(1 row)

\db shows it as in the CREATE TABLESPACE statement:

postgres=# \db+ ts

            List of tablespaces
 Name | Owner | Location | Access privileges |
                             Options
                         |  Size   | Description
------+-------+----------+-------------------+-----------------------------------------------------------------------------------------------
-------------------------+---------+-------------
 ts   | u1    | /tmp/ts  |                   |
{seq_page_cost=1.12345678910,random_page_cost=1.12345678910,effective_io_concurrency=17,mainte
nance_io_concurrency=18} | 0 bytes |
(1 row)


== permissions ==

Is it supposed to be visible to all users?

postgres=# CREATE USER u1;
CREATE ROLE
postgres=# CREATE TABLESPACE ts LOCATION '/tmp/ts';
CREATE TABLESPACE
postgres=# SET ROLE u1;
SET
postgres=> SELECT pg_get_tablespace_ddl('ts');
               pg_get_tablespace_ddl
----------------------------------------------------
 CREATE TABLESPACE ts OWNER jim LOCATION '/tmp/ts';
(1 row)

Note that \db does not allow it:

postgres=> SELECT CURRENT_USER;
 current_user
--------------
 u1
(1 row)

postgres=> \db+ ts
ERROR:  permission denied for tablespace ts


Best, Jim

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-03T23:49:23Z

    On Fri, Oct 31, 2025 at 10:36 AM Jim Jones <jim.jones@uni-muenster.de>
wrote:

> Hi Manni,
>
> Thanks for the patch!
>
> On 29/10/2025 02:23, Manni Wood wrote:
> > This patch creates a function pg_get_tablespace_ddl, designed to
> > retrieve the full DDL statement for a tablespace. Users can obtain the
> > DDL by providing the tablespace name, like so:
> >
> >     SELECT pg_get_tablespace_ddl('regress_owner_tblsp');
> >                                            pg_get_tablespace_ddl
> >
> >
> ---------------------------------------------------------------------------------------------------
> >      CREATE TABLESPACE regress_owner_tblsp OWNER regress_user LOCATION
> > '' WITH (random_page_cost = 3);
>
>
> Here my first comments regarding usability:
>
> == quoted identifier ==
>
> Tablespace names containing quoted identifiers cannot be parsed:
>
> postgres=# CREATE TABLESPACE "My TS" LOCATION '/tmp/ts';
> CREATE TABLESPACE
> postgres=# SELECT pg_get_tablespace_ddl('"My TS"');
> ERROR:  tablespace ""My TS"" does not exist
>
> The following works, but I guess it shouldn't:
>
> postgres=# SELECT pg_get_tablespace_ddl('My TS');
>              pg_get_tablespace_ddl
> -----------------------------------------------
>  CREATE TABLESPACE "My TS" LOCATION '/tmp/ts';
> (1 row)
>
> The same applies for unicode characters:
>
> postgres=# CREATE TABLESPACE "🐘" LOCATION '/tmp/ts';
> CREATE TABLESPACE
> postgres=# SELECT pg_get_tablespace_ddl('"🐘"');
> ERROR:  tablespace ""🐘"" does not exist
> postgres=# SELECT pg_get_tablespace_ddl('🐘');
>            pg_get_tablespace_ddl
> --------------------------------------------
>  CREATE TABLESPACE "🐘" LOCATION '/tmp/ts';
> (1 row)
>
>
> == option precision ==
>
> There is a precision loss in the options:
>
> postgres=# CREATE TABLESPACE ts OWNER u1 LOCATION '/tmp/ts' WITH
> (seq_page_cost = 1.12345678910, random_page_cost = 1.12345678910,
> effective_io_concurrency = 17, maintenance_io_concurrency = 18);
> CREATE TABLESPACE
> postgres=# SELECT pg_get_tablespace_ddl('ts');
>
>    pg_get_tablespace_ddl
>
>
> ---------------------------------------------------------------------------------------------------------------------------------------------
> ---------------------------------
>  CREATE TABLESPACE ts OWNER u1 LOCATION '/tmp/ts' WITH (random_page_cost
> = 1.12346, seq_page_cost = 1.12346, effective_io_concurrency = 17, m
> aintenance_io_concurrency = 18);
> (1 row)
>
> \db shows it as in the CREATE TABLESPACE statement:
>
> postgres=# \db+ ts
>
>             List of tablespaces
>  Name | Owner | Location | Access privileges |
>                              Options
>                          |  Size   | Description
>
> ------+-------+----------+-------------------+-----------------------------------------------------------------------------------------------
> -------------------------+---------+-------------
>  ts   | u1    | /tmp/ts  |                   |
>
> {seq_page_cost=1.12345678910,random_page_cost=1.12345678910,effective_io_concurrency=17,mainte
> nance_io_concurrency=18} | 0 bytes |
> (1 row)
>
>
> == permissions ==
>
> Is it supposed to be visible to all users?
>
> postgres=# CREATE USER u1;
> CREATE ROLE
> postgres=# CREATE TABLESPACE ts LOCATION '/tmp/ts';
> CREATE TABLESPACE
> postgres=# SET ROLE u1;
> SET
> postgres=> SELECT pg_get_tablespace_ddl('ts');
>                pg_get_tablespace_ddl
> ----------------------------------------------------
>  CREATE TABLESPACE ts OWNER jim LOCATION '/tmp/ts';
> (1 row)
>
> Note that \db does not allow it:
>
> postgres=> SELECT CURRENT_USER;
>  current_user
> --------------
>  u1
> (1 row)
>
> postgres=> \db+ ts
> ERROR:  permission denied for tablespace ts
>
>
> Best, Jim
>
>
> Hi, Jim

Thanks for reviewing my very first patch!

== quoted identifier ==

 I see that Postgres already has the SQL function has_tablespace_privilege
that behaves the same way as this patch's pg_get_tablespace_ddl.

# create tablespace "My TS" location '/tmp/has_space';
CREATE TABLESPACE

# select has_tablespace_privilege('My TS', 'create'); rollback;

┌──────────────────────────┐
│ has_tablespace_privilege │
├──────────────────────────┤
│ t                        │
└──────────────────────────┘
(1 row)

# select has_tablespace_privilege('"My TS"', 'create'); rollback;
ERROR:  42704: tablespace ""My TS"" does not exist


# create tablespace "🐘" location '/tmp/has_elephant';
CREATE TABLESPACE

# select has_tablespace_privilege('🐘', 'create'); rollback;
┌──────────────────────────┐
│ has_tablespace_privilege │
├──────────────────────────┤
│ t                        │
└──────────────────────────┘
(1 row)

# select has_tablespace_privilege('"🐘"', 'create'); rollback;
ERROR:  42704: tablespace ""🐘"" does not exist

Does the existence of this behavior in an existing function make the same
behavior less surprising for this patch's function?

== option precision ==

Thanks for pointing this out.

I have attached a v2 of the patch that just uses the original text the user
entered for the spcoptions.

This is much better, and it made the code smaller.

I have added "1.1234567890" to one of the tests to show that this works.

== permissions ==

I'm not sure what to think of this. psql's "\db+" does not let me show the
tablespace.

But if, as user 'u1', I select from pg_tablespace directly, I have the
permissions to do so:

postgres> select current_user; rollback;
┌──────────────┐
│ current_user │
├──────────────┤
│ u1           │
└──────────────┘
(1 row)

postgres> select * from pg_catalog.pg_tablespace; rollback;
┌───────┬────────────┬──────────┬────────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│  oid  │  spcname   │ spcowner │ spcacl │
                      spcoptions
            │
├───────┼────────────┼──────────┼────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│  1663 │ pg_default │       10 │ [NULL] │ [NULL]

          │
│  1664 │ pg_global  │       10 │ [NULL] │ [NULL]

          │
│ 19971 │ ts         │       10 │ [NULL] │
{seq_page_cost=1.12345678910,random_page_cost=1.12345678910,effective_io_concurrency=17,maintenance_io_concurrency=18}
│
└───────┴────────────┴──────────┴────────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
(3 rows)

So if the information is obtainable by selecting from
pg_catalog.pg_tablespace, it seems defensible to make the same data
available via pg_get_tablespace_ddl.

Thoughts?

Thanks again for reviewing my patch,

-Manni

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

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-04T08:27:59Z

    
On 04/11/2025 00:49, Manni Wood wrote:
> == quoted identifier ==
> 
>  I see that Postgres already has the SQL
> function has_tablespace_privilege that behaves the same way as this
> patch's pg_get_tablespace_ddl.

You're right. The source of my confusion is that I was approaching the
tablespace name as if it were a relation:

postgres=# CREATE TABLE "T"();
CREATE TABLE
postgres=# SELECT '"T"'::regclass::oid;
  oid
-------
 47766
(1 row)

postgres=# SELECT 'T'::regclass::oid;
ERROR:  relation "t" does not exist
LINE 1: SELECT 'T'::regclass::oid;

But I see that other functions behave similarly, e.g. pg_tablespace_size:

postgres=# SELECT pg_tablespace_size('My TS');
 pg_tablespace_size
--------------------
                  0
(1 row)

postgres=# SELECT pg_tablespace_size('"My TS"');
ERROR:  tablespace ""My TS"" does not exist
postgres=#

Sorry for the noise.

Do you think that an overload in pg_proc.dat with oid as parameter would
make sense here? e.g.

{ oid => '2322',
  descr => 'total disk space usage for the specified tablespace',
  proname => 'pg_tablespace_size', provolatile => 'v', prorettype => 'int8',
  proargtypes => 'oid', prosrc => 'pg_tablespace_size_oid' },
{ oid => '2323',
  descr => 'total disk space usage for the specified tablespace',
  proname => 'pg_tablespace_size', provolatile => 'v', prorettype => 'int8',
  proargtypes => 'name', prosrc => 'pg_tablespace_size_name' },


> 
> == option precision ==
> 
> Thanks for pointing this out.
> 
> I have attached a v2 of the patch that just uses the original text the
> user entered for the spcoptions.


Nice. It now shows the options without precision loss:

postgres=# CREATE TABLESPACE ts OWNER u1 LOCATION '/tmp/ts' WITH
(seq_page_cost = 1.12345678910, random_page_cost = 1.12345678910,
effective_io_concurrency = 17, maintenance_io_concurrency = 18);
CREATE TABLESPACE
postgres=# SELECT pg_get_tablespace_ddl('ts');

     pg_get_tablespace_ddl

-------------------------------------------------------------------------------------------------------------------------------------------
---------------------------------------
 CREATE TABLESPACE ts OWNER u1 LOCATION '/tmp/ts' WITH
(seq_page_cost=1.12345678910, random_page_cost=1.12345678910,
effective_io_concurrency=17, maintenance_io_concurrency=18);
(1 row)

postgres=# \db+ ts

            List of tablespaces
 Name | Owner | Location | Access privileges |
                             Options
                           |  Size   | Description
------+-------+----------+-------------------+---------------------------------------------------------------------------------------------
---------------------------+---------+-------------
 ts   | u1    | /tmp/ts  |                   |
{seq_page_cost=1.12345678910,random_page_cost=1.12345678910,effective_io_concurrency=17,maintenance_io_concurrency=18}
| 0 bytes |
(1 row)


> == permissions ==
> 
> I'm not sure what to think of this. psql's "\db+" does not let me show
> the tablespace.
> 

Right. I guess the difference here is that \db+ also shows the
tablespace's size, which requires the user to actually read it.

postgres=# CREATE TABLESPACE ts OWNER jim LOCATION '/tmp/ts';
CREATE TABLESPACE
postgres=# SELECT pg_tablespace_size('ts');
 pg_tablespace_size
--------------------
                  0
(1 row)

postgres=# SET ROLE u1;
SET
postgres=> SELECT pg_tablespace_size('ts');
ERROR:  permission denied for tablespace ts

Since pg_get_tablespace_ddl doesn't display size, I believe it's fine as-is.

Thanks.

Best, Jim

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

    Nishant Sharma <nishant.sharma@enterprisedb.com> — 2025-11-04T10:37:14Z

    On Tue, Nov 4, 2025 at 1:58 PM Jim Jones <jim.jones@uni-muenster.de> wrote:

>
> Do you think that an overload in pg_proc.dat with oid as parameter would
> make sense here? e.g.
>
> { oid => '2322',
>   descr => 'total disk space usage for the specified tablespace',
>   proname => 'pg_tablespace_size', provolatile => 'v', prorettype =>
> 'int8',
>   proargtypes => 'oid', prosrc => 'pg_tablespace_size_oid' },
> { oid => '2323',
>   descr => 'total disk space usage for the specified tablespace',
>   proname => 'pg_tablespace_size', provolatile => 'v', prorettype =>
> 'int8',
>   proargtypes => 'name', prosrc => 'pg_tablespace_size_name' },
>
> Using name as parameter is more user friendly than OID.
Because users usually do not know the oids. Constructing
the DDL from the name appears better as it contains a name
in it. So, no gain in having an OID version of
pg_get_tablespace_ddl.

PFA, v3 patch set. It has some cosmetic changes and few
improvements in the new code added by Manni in v2. Also, the
new test case added did not have a DROP statement for the
tablespace created, which caused make-world failure. So, I
corrected that too.


Regards,
Nishant Sharma.
EDB, Pune.

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

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-04T11:25:05Z

    Hi Nishant

On 04/11/2025 11:37, Nishant Sharma wrote:
> Using name as parameter is more user friendly than OID.
> Because users usually do not know the oids. Constructing
> the DDL from the name appears better as it contains a name
> in it. So, no gain in having an OID version of
> pg_get_tablespace_ddl.

Would you also say that having a pg_tablespace_size(oid) has no benefit?

I took a look at similar functions, and the only pattern I could
identify is that all of them take an oid parameter.

pg_tablespace_size:       oid and name
pg_tablespace_location:   oid
has_tablespace_privilege: oid, name, and text
pg_tablespace_databases:  oid
...
pg_get_tablespace_ddl:    name

I'm definitely not opposed to having just a name parameter, but I
thought it would be worth mentioning.

Best, Jim

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-04T14:19:38Z

    On Tue, Nov 4, 2025 at 5:25 AM Jim Jones <jim.jones@uni-muenster.de> wrote:

> Hi Nishant
>
> On 04/11/2025 11:37, Nishant Sharma wrote:
> > Using name as parameter is more user friendly than OID.
> > Because users usually do not know the oids. Constructing
> > the DDL from the name appears better as it contains a name
> > in it. So, no gain in having an OID version of
> > pg_get_tablespace_ddl.
>
> Would you also say that having a pg_tablespace_size(oid) has no benefit?
>
> I took a look at similar functions, and the only pattern I could
> identify is that all of them take an oid parameter.
>
> pg_tablespace_size:       oid and name
> pg_tablespace_location:   oid
> has_tablespace_privilege: oid, name, and text
> pg_tablespace_databases:  oid
> ...
> pg_get_tablespace_ddl:    name
>
> I'm definitely not opposed to having just a name parameter, but I
> thought it would be worth mentioning.
>
> Best, Jim
>

Hello,  Jim and Nishant!

About having an OID variant:

I definitely want to keep the current name-based parameter, and it looks
like we all agree on that.

The question is if we should additionally have an OID-based variant.

I personally see no harm in additionally having an OID-based variant,
seeing as it looks like a lot of functions do seem to take an OID. If I
understand correctly, many functions take an OID, and Postgres users are
supposed to have read the docs (
https://www.postgresql.org/docs/current/datatype-oid.html) to know to cast
names to OIDs. So, in terms of following established practice / patterns,
an OID-based variant is defensible.

Thankfully for people like me (for whom the "just cast the OID to a name"
pattern never sunk in after 25 years of using Postgres), I'm glad text/name
variants of functions are also a thing in Postgres, as I suspect every time
a user has a choice between the two, a user will choose to just provide the
name.

Let me know what you think!

Thanks, Jim,

Thanks Nishant for fixing/improving my v2 patch to v3!

-Manni

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

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-04T17:59:34Z

    
On 04/11/2025 15:19, Manni Wood wrote:
> I personally see no harm in additionally having an OID-based variant,
> seeing as it looks like a lot of functions do seem to take an OID. If I
> understand correctly, many functions take an OID, and Postgres users are
> supposed to have read the docs (https://www.postgresql.org/docs/current/
> datatype-oid.html <https://www.postgresql.org/docs/current/datatype-
> oid.html>) to know to cast names to OIDs. So, in terms of following
> established practice / patterns, an OID-based variant is defensible.

+1

That's the way I see it too. Of course it's always easier to use the
tablespace's name, but there might be cases where you only have the oid
-- in which case you'd need to do a JOIN with pg_tablespace to find the
name. It's just for convenience.

Best, Jim

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-04T19:26:48Z

    On 2025-Nov-04, Jim Jones wrote:

> That's the way I see it too. Of course it's always easier to use the
> tablespace's name, but there might be cases where you only have the oid
> -- in which case you'd need to do a JOIN with pg_tablespace to find the
> name. It's just for convenience.

The other DDL-producing patches that are being posted, all depend on a
reg* type for their argument, which means they will work correctly with
either an OID or an object name.  Tablespaces are one of the few object
types for which no "regtablespace" exists, so I think it's fair to
require both forms.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
[…] indem ich in meinem Leben oft an euch gedacht, euch glücklich zu machen. Seyd es!
A menudo he pensado en vosotros, en haceros felices. ¡Sedlo, pues!
		Heiligenstädter Testament, L. v. Beethoven, 1802
		https://de.wikisource.org/wiki/Heiligenstädter_Testament

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

    Nishant Sharma <nishant.sharma@enterprisedb.com> — 2025-11-05T07:32:07Z

    On Wed, Nov 5, 2025 at 12:56 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2025-Nov-04, Jim Jones wrote:
>
> > That's the way I see it too. Of course it's always easier to use the
> > tablespace's name, but there might be cases where you only have the oid
> > -- in which case you'd need to do a JOIN with pg_tablespace to find the
> > name. It's just for convenience.
>
> The other DDL-producing patches that are being posted, all depend on a
> reg* type for their argument, which means they will work correctly with
> either an OID or an object name.  Tablespaces are one of the few object
> types for which no "regtablespace" exists, so I think it's fair to
> require both forms.
>
> --
> Álvaro Herrera         PostgreSQL Developer  —
> https://www.EnterpriseDB.com/
> […] indem ich in meinem Leben oft an euch gedacht, euch glücklich zu
> machen. Seyd es!
> A menudo he pensado en vosotros, en haceros felices. ¡Sedlo, pues!
>                 Heiligenstädter Testament, L. v. Beethoven, 1802
>                 https://de.wikisource.org/wiki/Heiligenstädter_Testament



My reasons why I thought only name form was sufficient:-
1. The use case that I had in my mind for this get DDL function was
getting covered with name as its parameter. As we are creating DDL
and name will be part of it. Hence using it as input to our function to
create its DDL.
2. As Álvaro mentioned, we don't have any regtablespace for
tablespaces, So, using <tablespacename>::regtablespace::oid is not
a case for this get_ddl. But is valid for other get_ddl funcs. And even
for them we use the name in the form <objectname>::reg<object>::oid
and internally the get_ddl gets OID. The user again here does not
worry about the OIDs of their <objectname>.
3. As Manni mentions, regarding casting names to oid. But that is not
valid for tablespaces currently. If I am not missing anything. I think
users would explicitly need to provide OID to this function as a value
or from some "select oid ...".
4. The list of other tablespaces functions shared by Jim has two
functions, pg_tablespace_location() & pg_tablespace_databases()
that takes only oid as parameter and not name or text (maybe would
have been better), why? I am not sure, maybe the use case at that
time needed only an oid variant?

But yeah, with the current panel we have a majority here for having the
OID variant for this function. And of course there is no harm with it.

So, PFA v4 patch set. I have included the OID variant in it.


Regards,
Nishant Sharma.
EDB, Pune.
https://www.enterprisedb.com

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-05T08:21:31Z

    On 2025-Nov-05, Nishant Sharma wrote:

> My reasons why I thought only name form was sufficient:-
> 1. The use case that I had in my mind for this get DDL function was
> getting covered with name as its parameter. As we are creating DDL
> and name will be part of it. Hence using it as input to our function to
> create its DDL.

Accepting an OID as parameter lets the user call this function in a
query that returns a tablespace OID as parameter, without having to add
a join to pg_tablespace.  Not something you see very frequently, but I
can imagine GUI tool writers doing that.

> 4. The list of other tablespaces functions shared by Jim has two
> functions, pg_tablespace_location() & pg_tablespace_databases()
> that takes only oid as parameter and not name or text (maybe would
> have been better), why? I am not sure, maybe the use case at that
> time needed only an oid variant?

Lack of the other form of pg_tablespace_location has annoyed me on
occassion, but I don't think it's frequent enough to request it to be
added.  (I don't remember ever having a need to call
pg_tablespace_databases).

> +	/* Find tablespace directory path */
> +	datumlocation = DirectFunctionCall1(pg_tablespace_location, tspaceoid);
> +	path = text_to_cstring(DatumGetTextP(datumlocation));

It seems worth splitting pg_tablespace_location in two parts: one outer
shell that takes PG_FUNCTION_ARGS and returns text, and an inner
implementation function that takes a plain Oid and returns char *.  This
way, you can use the inner one here without the DirectFunctionCall1()
scaffolding, and avoid having to convert the laboriously constructed
text immediately back to a C string.

> +Datum
> +pg_get_tablespace_ddl_name(PG_FUNCTION_ARGS)
> +{
> +	PG_RETURN_TEXT_P(build_tablespace_ddl(get_tablespace_oid(NameStr(*(Name)PG_GETARG_NAME(0)),
> +															 false)));
> +}

This line is far too clever.  Better add a Name variable for
PG_GETARG_NAME(), an Oid variable for get_tablespace_oid(), and so on.
It'll be more code lines, but they will be ten times more readable.

> +Datum
> +pg_get_tablespace_ddl_oid(PG_FUNCTION_ARGS)
> +{
> +	PG_RETURN_TEXT_P(build_tablespace_ddl((Oid)PG_GETARG_OID(0)));
> +}

This one isn't _that_ bad -- still, our style is to have all the
PG_GETARG_foo() invocations together in an orderly fashion at the very
top of local variable declarations in pretty much all of our functions,
and I think that's a good habit to keep.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Digital and video cameras have this adjustment and film cameras don't for the
same reason dogs and cats lick themselves: because they can."   (Ken Rockwell)

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

    Nishant Sharma <nishant.sharma@enterprisedb.com> — 2025-11-05T11:53:57Z

    Thanks Álvaro for the review comments on v4!

PFA, v5 patch set. I have included all your review comments.


Regards,
Nishant Sharma.
EDB, Pune.
https://www.enterprisedb.com

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-06T22:08:11Z

    On Wed, Nov 5, 2025 at 5:54 AM Nishant Sharma <
nishant.sharma@enterprisedb.com> wrote:

> Thanks Álvaro for the review comments on v4!
>
> PFA, v5 patch set. I have included all your review comments.
>
>
> Regards,
> Nishant Sharma.
> EDB, Pune.
> https://www.enterprisedb.com
>

The BSD build was failing with the error 'WARNING:  tablespaces created by
regression test cases should have names starting with "regress_"', so the
attached patches should fix that.

The windows build is also failing, on this error
"../src/port/strerror.c(311): fatal error C1051: program database file,
'C:\cirrus\build\src\port\libpgport_srv.pdb', has an obsolete format,
delete it and recompile", which I don't think is related to our patch.
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-07T01:27:29Z

    On Thu, Nov 6, 2025 at 4:08 PM Manni Wood <manni.wood@enterprisedb.com>
wrote:

>
>
> On Wed, Nov 5, 2025 at 5:54 AM Nishant Sharma <
> nishant.sharma@enterprisedb.com> wrote:
>
>> Thanks Álvaro for the review comments on v4!
>>
>> PFA, v5 patch set. I have included all your review comments.
>>
>>
>> Regards,
>> Nishant Sharma.
>> EDB, Pune.
>> https://www.enterprisedb.com
>>
>
> The BSD build was failing with the error 'WARNING:  tablespaces created by
> regression test cases should have names starting with "regress_"', so the
> attached patches should fix that.
>
> The windows build is also failing, on this error
> "../src/port/strerror.c(311): fatal error C1051: program database file,
> 'C:\cirrus\build\src\port\libpgport_srv.pdb', has an obsolete format,
> delete it and recompile", which I don't think is related to our patch.
> --
> -- Manni Wood EDB: https://www.enterprisedb.com
>

And once again, I am foiled by whitespace. Attached v7 fixes problems in
tests due to whitespace.
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-07T09:58:31Z

    On 2025-Nov-05, Nishant Sharma wrote:

> Thanks Álvaro for the review comments on v4!
> 
> PFA, v5 patch set. I have included all your review comments.

Great, thanks.

I think adding the get_tablespace_location_string function in
ruleutils.c makes little sense -- I would say it belongs in
src/backend/catalog/pg_tablespace.c.  Since this file happens not to
exist, you can create it.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Someone said that it is at least an order of magnitude more work to do
production software than a prototype. I think he is wrong by at least
an order of magnitude."                              (Brian Kernighan)

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

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-07T16:16:38Z

    
On 07/11/2025 02:27, Manni Wood wrote:
> Attached v7 fixes problems in tests due to whitespace.


Since get_tablespace_loc_string returns a palloc'd string, I guess you
could pfree it after the if block. The same applies for spcowner, since
you're calling GetUserNameFromId() with noerr = false.

For reference, see pg_get_indexdef_worker():

...
/*
 * If it has options, append "WITH (options)"
 */
str = flatten_reloptions(indexrelid);
if (str)
{
  appendStringInfo(&buf, " WITH (%s)", str);
  pfree(str);
}
...


Thanks

Best, Jim

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-07T22:38:32Z

    On Fri, Nov 7, 2025 at 10:16 AM Jim Jones <jim.jones@uni-muenster.de> wrote:

>
>
> On 07/11/2025 02:27, Manni Wood wrote:
> > Attached v7 fixes problems in tests due to whitespace.
>
>
> Since get_tablespace_loc_string returns a palloc'd string, I guess you
> could pfree it after the if block. The same applies for spcowner, since
> you're calling GetUserNameFromId() with noerr = false.
>
> For reference, see pg_get_indexdef_worker():
>
> ...
> /*
>  * If it has options, append "WITH (options)"
>  */
> str = flatten_reloptions(indexrelid);
> if (str)
> {
>   appendStringInfo(&buf, " WITH (%s)", str);
>   pfree(str);
> }
> ...
>
>
> Thanks
>
> Best, Jim
>

Hello, Álvaro and Jim!

I have incorporated both of your suggestions into this pair of v8 patches.

Let me know what you think.
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-07T23:38:12Z

    On Fri, Nov 7, 2025 at 4:38 PM Manni Wood <manni.wood@enterprisedb.com>
wrote:

>
>
> On Fri, Nov 7, 2025 at 10:16 AM Jim Jones <jim.jones@uni-muenster.de>
> wrote:
>
>>
>>
>> On 07/11/2025 02:27, Manni Wood wrote:
>> > Attached v7 fixes problems in tests due to whitespace.
>>
>>
>> Since get_tablespace_loc_string returns a palloc'd string, I guess you
>> could pfree it after the if block. The same applies for spcowner, since
>> you're calling GetUserNameFromId() with noerr = false.
>>
>> For reference, see pg_get_indexdef_worker():
>>
>> ...
>> /*
>>  * If it has options, append "WITH (options)"
>>  */
>> str = flatten_reloptions(indexrelid);
>> if (str)
>> {
>>   appendStringInfo(&buf, " WITH (%s)", str);
>>   pfree(str);
>> }
>> ...
>>
>>
>> Thanks
>>
>> Best, Jim
>>
>
> Hello, Álvaro and Jim!
>
> I have incorporated both of your suggestions into this pair of v8 patches.
>
> Let me know what you think.
> --
> -- Manni Wood EDB: https://www.enterprisedb.com
>

Alas, the build https://commitfest.postgresql.org/patch/6175/ now fails,
and I cannot reproduce on my machine. Obviously there will be a v9...
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-08T00:03:08Z

    
On 08/11/2025 00:38, Manni Wood wrote:
> Alas, the build https://commitfest.postgresql.org/patch/6175/ <https://
> commitfest.postgresql.org/patch/6175/> now fails, and I cannot reproduce
> on my machine. Obviously there will be a v9...

You forgot the declaration for build_tablespace_ddl_string[1]:

ruleutils.c:13755:1: warning: no previous prototype for
‘build_tablespace_ddl_string’ [-Wmissing-prototypes]
13755 | build_tablespace_ddl_string(const Oid tspaceoid)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~

Best, Jim

1 - https://cirrus-ci.com/task/4855404196265984?logs=build#L1911

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-08T04:19:38Z

    On Fri, Nov 7, 2025 at 6:03 PM Jim Jones <jim.jones@uni-muenster.de> wrote:

>
>
> On 08/11/2025 00:38, Manni Wood wrote:
> > Alas, the build https://commitfest.postgresql.org/patch/6175/ <https://
> > commitfest.postgresql.org/patch/6175/> now fails, and I cannot reproduce
> > on my machine. Obviously there will be a v9...
>
> You forgot the declaration for build_tablespace_ddl_string[1]:
>
> ruleutils.c:13755:1: warning: no previous prototype for
> ‘build_tablespace_ddl_string’ [-Wmissing-prototypes]
> 13755 | build_tablespace_ddl_string(const Oid tspaceoid)
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Best, Jim
>
> 1 - https://cirrus-ci.com/task/4855404196265984?logs=build#L1911
>
>
Thank you very much, Jim. Serves me right for looking at the error at the
end of the logs rather than the warning in the middle.

v9 is attached.
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-08T04:46:28Z

    On Fri, Nov 7, 2025 at 10:19 PM Manni Wood <manni.wood@enterprisedb.com>
wrote:

>
>
> On Fri, Nov 7, 2025 at 6:03 PM Jim Jones <jim.jones@uni-muenster.de>
> wrote:
>
>>
>>
>> On 08/11/2025 00:38, Manni Wood wrote:
>> > Alas, the build https://commitfest.postgresql.org/patch/6175/ <https://
>> > commitfest.postgresql.org/patch/6175/> now fails, and I cannot
>> reproduce
>> > on my machine. Obviously there will be a v9...
>>
>> You forgot the declaration for build_tablespace_ddl_string[1]:
>>
>> ruleutils.c:13755:1: warning: no previous prototype for
>> ‘build_tablespace_ddl_string’ [-Wmissing-prototypes]
>> 13755 | build_tablespace_ddl_string(const Oid tspaceoid)
>>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Best, Jim
>>
>> 1 - https://cirrus-ci.com/task/4855404196265984?logs=build#L1911
>>
>>
> Thank you very much, Jim. Serves me right for looking at the error at the
> end of the logs rather than the warning in the middle.
>
> v9 is attached.
> --
> -- Manni Wood EDB: https://www.enterprisedb.com
>

Ah, the error at the end of the logs is indeed still happening. Glad to
have gotten rid of that earlier warning, though.

https://cirrus-ci.com/task/6437176629526528?logs=build#L1906-L1912

I will ask for Nishant's help with this and post another patch.
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-08T14:05:49Z

    On Fri, Nov 7, 2025 at 10:46 PM Manni Wood <manni.wood@enterprisedb.com>
wrote:

>
>
> On Fri, Nov 7, 2025 at 10:19 PM Manni Wood <manni.wood@enterprisedb.com>
> wrote:
>
>>
>>
>> On Fri, Nov 7, 2025 at 6:03 PM Jim Jones <jim.jones@uni-muenster.de>
>> wrote:
>>
>>>
>>>
>>> On 08/11/2025 00:38, Manni Wood wrote:
>>> > Alas, the build https://commitfest.postgresql.org/patch/6175/
>>> <https://
>>> > commitfest.postgresql.org/patch/6175/> now fails, and I cannot
>>> reproduce
>>> > on my machine. Obviously there will be a v9...
>>>
>>> You forgot the declaration for build_tablespace_ddl_string[1]:
>>>
>>> ruleutils.c:13755:1: warning: no previous prototype for
>>> ‘build_tablespace_ddl_string’ [-Wmissing-prototypes]
>>> 13755 | build_tablespace_ddl_string(const Oid tspaceoid)
>>>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Best, Jim
>>>
>>> 1 - https://cirrus-ci.com/task/4855404196265984?logs=build#L1911
>>>
>>>
>> Thank you very much, Jim. Serves me right for looking at the error at the
>> end of the logs rather than the warning in the middle.
>>
>> v9 is attached.
>> --
>> -- Manni Wood EDB: https://www.enterprisedb.com
>>
>
> Ah, the error at the end of the logs is indeed still happening. Glad to
> have gotten rid of that earlier warning, though.
>
> https://cirrus-ci.com/task/6437176629526528?logs=build#L1906-L1912
>
> I will ask for Nishant's help with this and post another patch.
> --
> -- Manni Wood EDB: https://www.enterprisedb.com
>

Apologies for the noise. Just wanted to confirm that I did a fresh clone of
github.com/postgres/postgres on my linux machine, applied both v9 patches
to master, ran
"configure", "make world", "make check-world", and everything passed.

However, I see that this
https://cirrus-ci.com/task/6437176629526528?logs=clone#L214 checks out a
specific commit on a specific branch:

"Checked out db131410131cb6a60f074213b0e7aaaa15d72f87 on cf/6175 branch."

My clone of postgres (which is presumably shallow?) does not show that
branch, not even with "git branch -r".

Thanks very much, all, for your patience.
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-08T16:30:57Z

    
On 08/11/2025 15:05, Manni Wood wrote:
> Apologies for the noise. Just wanted to confirm that I did a fresh clone
> of github.com/postgres/postgres <http://github.com/postgres/postgres> on
> my linux machine, applied both v9 patches to master, ran 
> "configure", "make world", "make check-world", and everything passed.


Perhaps a missing entry at src/backend/catalog/meson.build for
pg_tablespace.c? You probably don't see the error in your environment
because you're not building with meson.

Best, Jim

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

    Nishant Sharma <nishant.sharma@enterprisedb.com> — 2025-11-10T09:27:27Z

    1. I have moved our build_tablespace_ddl_string in pg_tablespace.c
2. Removed unnecessary includes in new file pg_tablespace.c
3. Added 'or oid' as type in doc file for documentation along with name.
4. Added 'pg_tablespace.c' in the meson build file.


The problem appears to be with meson build (also suggested by Jim):
https://cirrus-ci.com/task/5376297293053952

Hopefully this resolves the CI issue.

PFA, v10 patch set.


Regards,
Nishant Sharma.
EDB, Pune.
https://www.enterprisedb.com/

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-10T11:27:03Z

    On 2025-Nov-08, Manni Wood wrote:

> However, I see that this
> https://cirrus-ci.com/task/6437176629526528?logs=clone#L214 checks out a
> specific commit on a specific branch:
> 
> "Checked out db131410131cb6a60f074213b0e7aaaa15d72f87 on cf/6175 branch."
> 
> My clone of postgres (which is presumably shallow?) does not show that
> branch, not even with "git branch -r".

Yeah, these branches used by CI are made up on the spot and aren't
propagated out of that repository.  You can add a "remote" to your
existing clone,

git remote add cfbot https://github.com/postgresql-cfbot/postgresql.git

and then you can see the branches it creates and `git switch` to them.
Do mind that they are ephemeral, and for this thread they only contain
the patches you submitted yourself; really, this is only useful if
you're going to review other people's patches, and even then it might
still be better to use the patches from the mailing list (which is where
cfbot itself grabs them from anyway).

You should definitely be reviewing other people's patches, though, not
just because it's valuable for the community as a whole to have an
additional pair of eyes on them, but also because they can teach you
many things.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-10T13:47:01Z

    On Mon, Nov 10, 2025 at 5:27 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2025-Nov-08, Manni Wood wrote:
>
> > However, I see that this
> > https://cirrus-ci.com/task/6437176629526528?logs=clone#L214 checks out a
> > specific commit on a specific branch:
> >
> > "Checked out db131410131cb6a60f074213b0e7aaaa15d72f87 on cf/6175 branch."
> >
> > My clone of postgres (which is presumably shallow?) does not show that
> > branch, not even with "git branch -r".
>
> Yeah, these branches used by CI are made up on the spot and aren't
> propagated out of that repository.  You can add a "remote" to your
> existing clone,
>
> git remote add cfbot https://github.com/postgresql-cfbot/postgresql.git
>
> and then you can see the branches it creates and `git switch` to them.
> Do mind that they are ephemeral, and for this thread they only contain
> the patches you submitted yourself; really, this is only useful if
> you're going to review other people's patches, and even then it might
> still be better to use the patches from the mailing list (which is where
> cfbot itself grabs them from anyway).
>
> You should definitely be reviewing other people's patches, though, not
> just because it's valuable for the community as a whole to have an
> additional pair of eyes on them, but also because they can teach you
> many things.
>
> --
> Álvaro Herrera        Breisgau, Deutschland  —
> https://www.EnterpriseDB.com/
> "Oh, great altar of passive entertainment, bestow upon me thy discordant
> images
> at such speed as to render linear thought impossible" (Calvin a la TV)
>

Thank you very much, Nishant, Jim, and Álvaro!

I will start building with both make and Meson before submitting patches.
Lesson learned.

Also, Álvaro, yes, I do need to start reviewing patches this week.

Thanks, all!
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-11T15:16:08Z

    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)

  28. 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

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-12T16:15:46Z

    On 2025-Nov-12, Manni Wood wrote:

> Thanks, Álvaro, for your continued help with this.
> 
> I have attached v11 patches that use all of the fixes from your
> review.patch.txt.

OK, thanks, I pushed 0001 now.

I think you could claim that some routines currently in
src/backend/commands/tablespace.c logically belong in the new file, but
unless you want to take on the task of moving a lot of other routines
under commands/ to their respective catalog/ file, then I think it's
more or less fine as is.

To be clear, I do not intend to do anything with your 0002 patch [for
now].  I'm going to let Andrew take these DDL-producing functions in his
hands.  Here I'm just posting your 0002 again, to make the cfbot happy.

Thanks

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-12T16:33:33Z

    On Wed, Nov 12, 2025 at 10:15 AM Álvaro Herrera <alvherre@kurilemu.de>
wrote:

> On 2025-Nov-12, Manni Wood wrote:
>
> > Thanks, Álvaro, for your continued help with this.
> >
> > I have attached v11 patches that use all of the fixes from your
> > review.patch.txt.
>
> OK, thanks, I pushed 0001 now.
>
> I think you could claim that some routines currently in
> src/backend/commands/tablespace.c logically belong in the new file, but
> unless you want to take on the task of moving a lot of other routines
> under commands/ to their respective catalog/ file, then I think it's
> more or less fine as is.
>
> To be clear, I do not intend to do anything with your 0002 patch [for
> now].  I'm going to let Andrew take these DDL-producing functions in his
> hands.  Here I'm just posting your 0002 again, to make the cfbot happy.
>
> Thanks
>
> --
> Álvaro Herrera               48°01'N 7°57'E  —
> https://www.EnterpriseDB.com/
> "Nunca se desea ardientemente lo que solo se desea por razón" (F.
> Alexandre)
>

OK, thanks very much, Álvaro.

If you are OK with the current state of the patch, then I am happy to not
move any more functions into their respective catalog/ files. My co-author,
Nishant, should feel free to offer his opinion here too.

--
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Chao Li <li.evan.chao@gmail.com> — 2025-11-13T06:06:44Z

    Hi Manni,

I just reviewed and tested the patch, just got a few small comments:

> On Nov 13, 2025, at 00:15, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> On 2025-Nov-12, Manni Wood wrote:
> 
>> Thanks, Álvaro, for your continued help with this.
>> 
>> I have attached v11 patches that use all of the fixes from your
>> review.patch.txt.
> 
> OK, thanks, I pushed 0001 now.
> 
> I think you could claim that some routines currently in
> src/backend/commands/tablespace.c logically belong in the new file, but
> unless you want to take on the task of moving a lot of other routines
> under commands/ to their respective catalog/ file, then I think it's
> more or less fine as is.
> 
> To be clear, I do not intend to do anything with your 0002 patch [for
> now].  I'm going to let Andrew take these DDL-producing functions in his
> hands.  Here I'm just posting your 0002 again, to make the cfbot happy.
> 
> Thanks
> 
> -- 
> Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
> "Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)
> <v12-0002-Adds-pg_get_tablespace_ddl-function.patch>


1.
```
+				 errmsg("tablespace with oid %d does not exist",
```

Existing code all use “%u” to format oid. You may search for “oid %” to see that.

2.
```
+			/* Add the options in WITH clause */
+			appendStringInfoString(&buf, TextDatumGetCString(optdatums[i]));
+			appendStringInfoString(&buf, ", “);
```

This two statements can be combined into one:

appendStringInfoString(&buf,  “%s, “,  TextDatumGetCString(optdatums[i]));

3
```
+		if (strncmp(PG_TBLSPC_DIR_SLASH, path, strlen(PG_TBLSPC_DIR_SLASH)) == 0)
+			appendStringInfoString(&buf, " LOCATION ''");
+		else
+			appendStringInfo(&buf, " LOCATION '%s'", path);
+	}
```

Instead of hardcoding single-quotes, we can use quote_literal_cstr().

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-18T14:15:03Z

    On Thu, Nov 13, 2025 at 12:07 AM Chao Li <li.evan.chao@gmail.com> wrote:

> Hi Manni,
>
> I just reviewed and tested the patch, just got a few small comments:
>
> > On Nov 13, 2025, at 00:15, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >
> > On 2025-Nov-12, Manni Wood wrote:
> >
> >> Thanks, Álvaro, for your continued help with this.
> >>
> >> I have attached v11 patches that use all of the fixes from your
> >> review.patch.txt.
> >
> > OK, thanks, I pushed 0001 now.
> >
> > I think you could claim that some routines currently in
> > src/backend/commands/tablespace.c logically belong in the new file, but
> > unless you want to take on the task of moving a lot of other routines
> > under commands/ to their respective catalog/ file, then I think it's
> > more or less fine as is.
> >
> > To be clear, I do not intend to do anything with your 0002 patch [for
> > now].  I'm going to let Andrew take these DDL-producing functions in his
> > hands.  Here I'm just posting your 0002 again, to make the cfbot happy.
> >
> > Thanks
> >
> > --
> > Álvaro Herrera               48°01'N 7°57'E  —
> https://www.EnterpriseDB.com/
> > "Nunca se desea ardientemente lo que solo se desea por razón" (F.
> Alexandre)
> > <v12-0002-Adds-pg_get_tablespace_ddl-function.patch>
>
>
> 1.
> ```
> +                                errmsg("tablespace with oid %d does not
> exist",
> ```
>
> Existing code all use “%u” to format oid. You may search for “oid %” to
> see that.
>
> 2.
> ```
> +                       /* Add the options in WITH clause */
> +                       appendStringInfoString(&buf,
> TextDatumGetCString(optdatums[i]));
> +                       appendStringInfoString(&buf, ", “);
> ```
>
> This two statements can be combined into one:
>
> appendStringInfoString(&buf,  “%s, “,  TextDatumGetCString(optdatums[i]));
>
> 3
> ```
> +               if (strncmp(PG_TBLSPC_DIR_SLASH, path,
> strlen(PG_TBLSPC_DIR_SLASH)) == 0)
> +                       appendStringInfoString(&buf, " LOCATION ''");
> +               else
> +                       appendStringInfo(&buf, " LOCATION '%s'", path);
> +       }
> ```
>
> Instead of hardcoding single-quotes, we can use quote_literal_cstr().
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
>
Hello, Chao Li!

Thank you for the improvements to my patch, helping it follow more of
Postgres's coding conventions. Much appreciated.

I have attached v13 of the patch. Now that Álvaro has merged the contents
of the 0001 patch, I assume this v13 patch can be 0001 and not 0002.
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Chao Li <li.evan.chao@gmail.com> — 2025-11-19T03:13:00Z

    Hi Manni,

I just reviewed v13 again, and still got a couple of comments:

> On Nov 18, 2025, at 22:15, Manni Wood <manni.wood@enterprisedb.com> wrote:
> 
> 
> <v13-0001-Adds-pg_get_tablespace_ddl-function.patch>

1. Do we need to perform some privilege check? I just did a test:
```
evantest=> \c
You are now connected to database "evantest" as user "evan".
evantest=> select pg_get_tablespace_ddl('pg_default');
           pg_get_tablespace_ddl
-------------------------------------------
 CREATE TABLESPACE pg_default OWNER chaol;
(1 row)
```

Where “evan” is a new user without grant any persuasion to it, but it can view the system default tablespace’s DDL. I don’t think that’s expected.

2. 
```
+		optarray = DatumGetArrayTypeP(datum);
+
+		deconstruct_array_builtin(optarray, TEXTOID,
+								  &optdatums, NULL, &optcount);
+
+		Assert(optcount);
+
+		/* Start WITH clause */
+		appendStringInfoString(&buf, " WITH (");
+
+		for (i = 0; i < (optcount - 1); i++)	/* Skipping last option */
+		{
+			/* Add the options in WITH clause */
+			appendStringInfo(&buf, "%s, ", TextDatumGetCString(optdatums[i]));
+		}
+
+		/* Adding the last remaining option */
+		appendStringInfoString(&buf, TextDatumGetCString(optdatums[i]));
```

This block of code is a duplicate of get_reloptions() defined in ruleutils.c, and get_reloptions() performs more checks. So I think build_tablespace_ddl_string() should be defined in ruleutils.c and reuse the existing helper function.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Jim Jones <jim.jones@uni-muenster.de> — 2025-11-19T07:52:49Z

    Hi Chao

On 19/11/2025 04:13, Chao Li wrote:
> 1. Do we need to perform some privilege check? I just did a test:
> ```
> evantest=> \c
> You are now connected to database "evantest" as user "evan".
> evantest=> select pg_get_tablespace_ddl('pg_default');
>            pg_get_tablespace_ddl
> -------------------------------------------
>  CREATE TABLESPACE pg_default OWNER chaol;
> (1 row)
> ```
> 
> Where “evan” is a new user without grant any persuasion to it, but it can view the system default tablespace’s DDL. I don’t think that’s expected.

It is expected. \db behaves similarly:


CREATE TABLESPACE ts LOCATION '/tmp/ts';
CREATE TABLESPACE

postgres=# CREATE USER foo;
CREATE ROLE

postgres=# SET ROLE foo;
SET

postgres=> \db ts
   List of tablespaces
 Name | Owner | Location
------+-------+----------
 ts   | jim   | /tmp/ts
(1 row)


IIUC the user foo is just reading the catalog entry of the new
tablespace, which is fine. Of course, accessing the tablespace itself is
not allowed. See \db+ (calculates the tablespace's size)


postgres=> \db+ ts
ERROR:  permission denied for tablespace ts

Best, Jim

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

    Manni Wood <manni.wood@enterprisedb.com> — 2025-11-19T22:52:12Z

    On Wed, Nov 19, 2025 at 1:52 AM Jim Jones <jim.jones@uni-muenster.de> wrote:

>
> Hi Chao
>
> On 19/11/2025 04:13, Chao Li wrote:
> > 1. Do we need to perform some privilege check? I just did a test:
> > ```
> > evantest=> \c
> > You are now connected to database "evantest" as user "evan".
> > evantest=> select pg_get_tablespace_ddl('pg_default');
> >            pg_get_tablespace_ddl
> > -------------------------------------------
> >  CREATE TABLESPACE pg_default OWNER chaol;
> > (1 row)
> > ```
> >
> > Where “evan” is a new user without grant any persuasion to it, but it
> can view the system default tablespace’s DDL. I don’t think that’s expected.
>
> It is expected. \db behaves similarly:
>
>
> CREATE TABLESPACE ts LOCATION '/tmp/ts';
> CREATE TABLESPACE
>
> postgres=# CREATE USER foo;
> CREATE ROLE
>
> postgres=# SET ROLE foo;
> SET
>
> postgres=> \db ts
>    List of tablespaces
>  Name | Owner | Location
> ------+-------+----------
>  ts   | jim   | /tmp/ts
> (1 row)
>
>
> IIUC the user foo is just reading the catalog entry of the new
> tablespace, which is fine. Of course, accessing the tablespace itself is
> not allowed. See \db+ (calculates the tablespace's size)
>
>
> postgres=> \db+ ts
> ERROR:  permission denied for tablespace ts
>
> Best, Jim
>

Hello, Chao.

Thanks as always for your ongoing help with improving this feature.

Instead of moving build_tablespace_ddl_string out of pg_tablespace.c, I
made get_reloptions visible outside of ruleutils.c.

Otherwise, I followed your advice on using get_reloptions to DRY up the
code.

Let me know what you think!
-- 
-- Manni Wood EDB: https://www.enterprisedb.com

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

    Nishant Sharma <nishant.sharma@enterprisedb.com> — 2025-11-26T06:05:04Z

    Thanks Manni for the v14 patch!

Thanks all for the review comments!

I think we are in position to move to 'Ready for Committer'
status.
CFBot is all green for v14:
https://commitfest.postgresql.org/patch/6175/


Hi Andrew,

We are moving this thread to 'Ready for Committer' for your
reviews and finalisation of the patch. Thanks!


Regards,
Nishant Sharma.
EDB, Pune.
https://www.enterprisedb.com/

>