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

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

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

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

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.

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

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

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

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

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

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)

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

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

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

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)

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

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

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

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

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