pg_get__*_ddl consolidation

On Fri, 20 Mar 2026 at 00:04, Andrew Dunstan <andrew@dunslane.net> wrote:

Greetings

Euler Taveira and I have been working on consolidating these patches.

These patches came out of a suggestion from me some time back [1], and I
used it as the base for some work at an EDB internal program. Perhaps I
was motivated a bit by Mao's dictum "Let a hundred flowers bloom; let a
hundred schools of thought contend." I wanted to see what people would
come up with. Therefore, if this has seemed a bit chaotic, I apologize,
both to the authors and to the list. I won't do things quite this way in
future.

Rather than adding to the already huge ruleutils.c, we decided to create
a new ddlutils.c file to contain these functions and their associated
infrastructure. There is in fact a fairly clean separation between these
functions and ruleutils. We just need to expose one function in ruleutils.

We (Euler and I) decided to concentrate on setting up common
infrastucture and ensuring a common argument and result structure. In
this first round, we are proposing to add functions for getting the DDL
for databases, tablespaces, and roles. We decided to stop there for now.
This sets up a good basis for dealing with more object types in future.
To the authors of the remaining patches - rest assured you have not been
forgotten.

Patch 1 sets up the functions used by the rest for option parsing. see [2]
Patch 2 implements pg_get_role_dll see[3]
Patch 3 implements pg_get_tabespace_ddl see [4]
Patch 4 implements pg_get_database_ddl see [2]

cheers

andrew

[1]
/messages/by-id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9@dunslane.net

[2]
/messages/by-id/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com

[3]
/messages/by-id/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com

[4]
/messages/by-id/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi all,
I was reading these patches and found that any user can get the
definition of database/roles by pg_get__*_ddl. I think these functions
should be restricted only to super users as these are cluster level
objects.

TAB is not working for these functions. I think these functions
should be displayed with TAB. I will do some more study and will do
some more tests.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On 2026-03-19 Th 3:55 PM, Mahendra Singh Thalor wrote:

On Fri, 20 Mar 2026 at 00:04, Andrew Dunstan<andrew@dunslane.net> wrote:

Greetings

Euler Taveira and I have been working on consolidating these patches.

These patches came out of a suggestion from me some time back [1], and I
used it as the base for some work at an EDB internal program. Perhaps I
was motivated a bit by Mao's dictum "Let a hundred flowers bloom; let a
hundred schools of thought contend." I wanted to see what people would
come up with. Therefore, if this has seemed a bit chaotic, I apologize,
both to the authors and to the list. I won't do things quite this way in
future.

Rather than adding to the already huge ruleutils.c, we decided to create
a new ddlutils.c file to contain these functions and their associated
infrastructure. There is in fact a fairly clean separation between these
functions and ruleutils. We just need to expose one function in ruleutils.

We (Euler and I) decided to concentrate on setting up common
infrastucture and ensuring a common argument and result structure. In
this first round, we are proposing to add functions for getting the DDL
for databases, tablespaces, and roles. We decided to stop there for now.
This sets up a good basis for dealing with more object types in future.
To the authors of the remaining patches - rest assured you have not been
forgotten.

Patch 1 sets up the functions used by the rest for option parsing. see [2]
Patch 2 implements pg_get_role_dll see[3]
Patch 3 implements pg_get_tabespace_ddl see [4]
Patch 4 implements pg_get_database_ddl see [2]

cheers

andrew

[1]
/messages/by-id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9@dunslane.net

[2]
/messages/by-id/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com

[3]
/messages/by-id/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com

[4]
/messages/by-id/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Hi all,
I was reading these patches and found that any user can get the
definition of database/roles by pg_get__*_ddl. I think these functions
should be restricted only to super users as these are cluster level
objects.

You could construct these functions using plpgsql. The information isn't
hidden from non-superusers. So what exactly would making these functions
superuser-only achieve? Now it's true that the user might not be able to
execute the DDL. But that's not the point.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Fri, 20 Mar 2026 at 01:44, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2026-03-19 Th 3:55 PM, Mahendra Singh Thalor wrote:

On Fri, 20 Mar 2026 at 00:04, Andrew Dunstan <andrew@dunslane.net> wrote:

Greetings

Euler Taveira and I have been working on consolidating these patches.

These patches came out of a suggestion from me some time back [1], and I
used it as the base for some work at an EDB internal program. Perhaps I
was motivated a bit by Mao's dictum "Let a hundred flowers bloom; let a
hundred schools of thought contend." I wanted to see what people would
come up with. Therefore, if this has seemed a bit chaotic, I apologize,
both to the authors and to the list. I won't do things quite this way in
future.

Rather than adding to the already huge ruleutils.c, we decided to create
a new ddlutils.c file to contain these functions and their associated
infrastructure. There is in fact a fairly clean separation between these
functions and ruleutils. We just need to expose one function in ruleutils.

We (Euler and I) decided to concentrate on setting up common
infrastucture and ensuring a common argument and result structure. In
this first round, we are proposing to add functions for getting the DDL
for databases, tablespaces, and roles. We decided to stop there for now.
This sets up a good basis for dealing with more object types in future.
To the authors of the remaining patches - rest assured you have not been
forgotten.

Patch 1 sets up the functions used by the rest for option parsing. see [2]
Patch 2 implements pg_get_role_dll see[3]
Patch 3 implements pg_get_tabespace_ddl see [4]
Patch 4 implements pg_get_database_ddl see [2]

cheers

andrew

[1]
/messages/by-id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9@dunslane.net

[2]
/messages/by-id/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com

[3]
/messages/by-id/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com

[4]
/messages/by-id/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi all,
I was reading these patches and found that any user can get the
definition of database/roles by pg_get__*_ddl. I think these functions
should be restricted only to super users as these are cluster level
objects.

You could construct these functions using plpgsql. The information isn't hidden from non-superusers. So what exactly would making these functions superuser-only achieve? Now it's true that the user might not be able to execute the DDL. But that's not the point.

Thanks Andrew for the clarification.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Sorry, my question might be stupid. Can we use these functions
directly into our dump utilities so that common code can be removed
and if there is any syntax change for a particular object, then no
need to change into different-2 places, rather we just need to change
it into ddlutils.c file only.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On 2026-03-19 Th 4:28 PM, Mahendra Singh Thalor wrote:

On Fri, 20 Mar 2026 at 01:44, Andrew Dunstan <andrew@dunslane.net> wrote:

You could construct these functions using plpgsql. The information isn't hidden from non-superusers. So what exactly would making these functions superuser-only achieve? Now it's true that the user might not be able to execute the DDL. But that's not the point.

Thanks Andrew for the clarification.

Sorry, my question might be stupid. Can we use these functions
directly into our dump utilities so that common code can be removed
and if there is any syntax change for a particular object, then no
need to change into different-2 places, rather we just need to change
it into ddlutils.c file only.

Well, we have tried to make the output like pg_dump. We might later
provide options for more compact output. But it remains to be seen if it
can be used in the pg_dump utilities. After all, it is going to produce
output suitable for the source version, since there is no knowledge in
the server of future versions. But pg_dump emits SQL suitable for the
target version.

My original motivation was quite different. It was to give the user a
piece of SQL that they could modify as they saw fit, rather than making
them start with a blank canvas.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hello!

I found a few problematic corner cases while testing the patches,
please look at the following:

Doesn't pg_get_database_ddl need more filtering for roles?

See example:

CREATE DATABASE testdb;
CREATE ROLE testrole;
ALTER DATABASE testdb SET work_mem TO '256MB';
ALTER ROLE testrole IN DATABASE testdb SET work_mem TO '512MB';
SELECT pg_get_database_ddl('testdb');

Another issue is that the data style isn't fixed:

CREATE ROLE regress_datestyle_test VALID UNTIL '2030-12-31 23:59:59+00';
SET DateStyle TO 'SQL, DMY';
SELECT * FROM pg_get_role_ddl('regress_datestyle_test');
-- returned statement fails with invalid input syntax for timestamp

+ appendStringInfo(&buf, "ALTER DATABASE %s OWNER = %s;",
+ dbname, quote_identifier(owner));

Shouldn't that be OWNER TO? Similarly this will result in an error
when executed.

Role memberships seem to be missing. I would expect those to be included?

CREATE ROLE regress_parent;
CREATE ROLE regress_child;
GRANT regress_parent TO regress_child;
SELECT * FROM pg_get_role_ddl('regress_child');

+ dbname = quote_identifier(NameStr(dbform->datname));

Isn't an pstrdup missing from here? dbname is used after ReleaseSysCache.

On Thu, Mar 19, 2026, at 6:32 PM, Zsolt Parragi wrote:

I found a few problematic corner cases while testing the patches,
please look at the following:

Thanks for testing.

Doesn't pg_get_database_ddl need more filtering for roles?

See example:

CREATE DATABASE testdb;
CREATE ROLE testrole;
ALTER DATABASE testdb SET work_mem TO '256MB';
ALTER ROLE testrole IN DATABASE testdb SET work_mem TO '512MB';
SELECT pg_get_database_ddl('testdb');

That's a design flaw. The goal is to returns SQL commands to reconstruct the
object. Since I don't know if you will use the testrole role to create testdb
database or even if the testrole exists in the cluster, it shouldn't return the
ALTER DATABASE testdb SET work_mem TO '512MB' (because that property belongs to
testrole role). It should only consider cases that setrole = 0 (as the comment
said). (This is not in the last patch [1]/messages/by-id/CANxoLDfLDa6Oh9y=QtVoCd=03b9ydeFF6fUe8vR1wPYU7refBg@mail.gmail.com but the output of these pg_get_*_ddl
functions should be as close as possible to pg_dump(all) output. There is
another discussion [2]/messages/by-id/202603021736.6nix27wwg6e6@alvherre.pgsql that asks how to test these functions; one way is to
compare the output with pg_dump(all). Another point is that these functions can
be used by a dump tool.)

Another issue is that the data style isn't fixed:

CREATE ROLE regress_datestyle_test VALID UNTIL '2030-12-31 23:59:59+00';
SET DateStyle TO 'SQL, DMY';
SELECT * FROM pg_get_role_ddl('regress_datestyle_test');
-- returned statement fails with invalid input syntax for timestamp

I couldn't reproduce the failure. The non-fixed DateStyle is by design. It
mimics pg_dumpall.

$ psql -d postgres
psql (19devel)
Type "help" for help.

postgres=# CREATE ROLE regress_datestyle_test VALID UNTIL '2030-12-31 23:59:59+00';
CREATE ROLE
postgres=# SHOW DateStyle;
DateStyle
-----------
ISO, DMY
(1 row)

postgres=# SET DateStyle TO 'SQL, DMY';
SET
postgres=# SELECT * FROM pg_get_role_ddl('regress_datestyle_test');
pg_get_role_ddl
---------------------------------------------------------------------------------------------------------------------------------------------------------
CREATE ROLE regress_datestyle_test NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN NOREPLICATION NOBYPASSRLS VALID UNTIL '31/12/2030 20:59:59 -03';
(1 row)

postgres=# CREATE ROLE regress_datestyle_test2 NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB NOLOGIN NOREPLICATION NOBYPASSRLS VALID UNTIL '31/12/2030 20:59:59 -03';
CREATE ROLE
postgres=# \du
List of roles
Role name | Attributes
-------------------------+------------------------------------------------------------
euler | Superuser, Create role, Create DB, Replication, Bypass RLS
regress_datestyle_test | Cannot login +
| Password valid until 31/12/2030 20:59:59 -03
regress_datestyle_test2 | Cannot login +
| Password valid until 31/12/2030 20:59:59 -03

+ appendStringInfo(&buf, "ALTER DATABASE %s OWNER = %s;",
+ dbname, quote_identifier(owner));

Shouldn't that be OWNER TO? Similarly this will result in an error
when executed.

Ooops. Let me get my brown paper bag.

Role memberships seem to be missing. I would expect those to be included?

CREATE ROLE regress_parent;
CREATE ROLE regress_child;
GRANT regress_parent TO regress_child;
SELECT * FROM pg_get_role_ddl('regress_child');

That's a good suggestion. Using the same argument as the first question, you
don't know if regress_parent role exists in the other cluster. I'm not opposed
to your idea but IMO it should be an option.

+ dbname = quote_identifier(NameStr(dbform->datname));

Isn't an pstrdup missing from here? dbname is used after ReleaseSysCache.

Yes. I missed this point while adding pg_db_role_setting code.

[1]: /messages/by-id/CANxoLDfLDa6Oh9y=QtVoCd=03b9ydeFF6fUe8vR1wPYU7refBg@mail.gmail.com
[2]: /messages/by-id/202603021736.6nix27wwg6e6@alvherre.pgsql

--
Euler Taveira
EDB https://www.enterprisedb.com/

On 2026-Mar-19, Andrew Dunstan wrote:

Greetings

Euler Taveira and I have been working on consolidating these patches.

Hmm, did you remove the permissions checking to dump objects? I thought
we had concluded that these were needed -- ie. you have to have at least
CONNECT to a database to be able to dump it, and so on. This way, the
functions do not override a DBAs intention to hide the information, when
they run REVOKE on the catalogs. I know this is a nonstandard thing to
do, but some people do it nonetheless.

/messages/by-id/202511131446.uzn4c25ljmd4@alvherre.pgsql

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Computing is too important to be left to men." (Karen Spärck Jones)

On 2026-03-20 Fr 9:15 AM, Álvaro Herrera wrote:

On 2026-Mar-19, Andrew Dunstan wrote:

Greetings

Euler Taveira and I have been working on consolidating these patches.

Hmm, did you remove the permissions checking to dump objects? I thought
we had concluded that these were needed -- ie. you have to have at least
CONNECT to a database to be able to dump it, and so on. This way, the
functions do not override a DBAs intention to hide the information, when
they run REVOKE on the catalogs. I know this is a nonstandard thing to
do, but some people do it nonetheless.

/messages/by-id/202511131446.uzn4c25ljmd4@alvherre.pgsql

Oh, hmm, yes, I think we did. Will work on it.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi, Andrew

On Thu, 19 Mar 2026 at 14:34, Andrew Dunstan <andrew@dunslane.net> wrote:

Greetings

Euler Taveira and I have been working on consolidating these patches.

These patches came out of a suggestion from me some time back [1], and
I used it as the base for some work at an EDB internal
program. Perhaps I was motivated a bit by Mao's dictum "Let a hundred
flowers bloom; let a hundred schools of thought contend." I wanted to
see what people would come up with. Therefore, if this has seemed a
bit chaotic, I apologize, both to the authors and to the list. I won't
do things quite this way in future.

Rather than adding to the already huge ruleutils.c, we decided to
create a new ddlutils.c file to contain these functions and their
associated infrastructure. There is in fact a fairly clean separation
between these functions and ruleutils. We just need to expose one
function in ruleutils.

We (Euler and I) decided to concentrate on setting up common
infrastucture and ensuring a common argument and result structure. In
this first round, we are proposing to add functions for getting the
DDL for databases, tablespaces, and roles. We decided to stop there
for now. This sets up a good basis for dealing with more object types
in future. To the authors of the remaining patches - rest assured you
have not been forgotten.

Patch 1 sets up the functions used by the rest for option parsing. see [2]
Patch 2 implements pg_get_role_dll see[3]
Patch 3 implements pg_get_tabespace_ddl see [4]
Patch 4 implements pg_get_database_ddl see [2]

Thanks for updating the patches. Here are some initial comments.

Patch 1
=======

1.
+       DDL_OPT_INT
+} DdlOptType;

A trailing comma should be added to match our coding conventions.

2.
+       bool            boolval;                /* filled in for DDL_OPT_BOOL */
+       char       *textval;            /* filled in for DDL_OPT_TEXT (palloc'd) */
+       int                     intval;                 /* filled in for DDL_OPT_INT */

Is it feasible to use a union for these three fields?

3.
+append_ddl_option(StringInfo buf, bool pretty, int indent,
+                                 const char *fmt,...)
+{
+       va_list         args;

IMO, limiting the variable scope to the for loop would be better.

Patch 2
=======

1.
+                foreach(lc, namelist)
+                {
+                    char       *curname = (char *) lfirst(lc);
+
+                    appendStringInfoString(&buf, quote_literal_cstr(curname));
+                    if (lnext(namelist, lc))
+                        appendStringInfoString(&buf, ", ");
+                }

We can use a boolean variable, such as first, to avoid calling lnext(),
and then replace foreach() with foreach_ptr().

2.
+       if (funcctx->call_cntr < funcctx->max_calls)
+       {
+               char       *stmt;
+
+               lc = list_nth_cell(statements, funcctx->call_cntr);
+               stmt = (char *) lfirst(lc);
+
+               SRF_RETURN_NEXT(funcctx, CStringGetTextDatum(stmt));
+       }

Could we use list_nth() in a similar manner to patch 3?

Patch 4
=======

Same as patch 2.

cheers

andrew

[1]
/messages/by-id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9@dunslane.net

[2]
/messages/by-id/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com

[3]
/messages/by-id/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com

[4]
/messages/by-id/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

[2. text/x-patch; 0001-Add-DDL-option-parsing-infrastructure-for-pg_get_-_d.patch]...

[3. text/x-patch; 0002-Add-pg_get_role_ddl-function.patch]...

[4. text/x-patch; 0004-Add-pg_get_database_ddl-function.patch]...

[5. text/x-patch; 0003-Add-pg_get_tablespace_ddl-function.patch]...

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

I couldn't reproduce the failure. The non-fixed DateStyle is by design. It
mimics pg_dumpall.

Sorry, I made a mistake while only copying the relevant parts of the
test case here. This is the correct test case:

SET DateStyle TO 'SQL, DMY'; -- change from default MDY to DMY
CREATE ROLE regress_datestyle_test VALID UNTIL '2030-12-31 23:59:59+00';
SELECT * FROM pg_get_role_ddl('regress_datestyle_test');
DROP ROLE regress_datestyle_test;
SET DateStyle TO 'SQL, MDY'; -- go back to default, or open a new
connection to use the generated command
-- try to run statement generated by pg_get_role_ddl

I'm not saying that this is necessarily wrong, but perhaps it's worth
mentioning somewhere?

Since I don't know if you will use the testrole role to create testdb
database or even if the testrole exists in the cluster, it shouldn't return the
ALTER DATABASE testdb SET work_mem TO '512MB' (because that property belongs to
testrole role).

Sorry, I wasn't specific previously, it returns the role specific
role, without the role specific condition:

CREATE DATABASE testdb;
CREATE ROLE testrole;
ALTER DATABASE testdb SET work_mem TO '256MB';
ALTER ROLE testrole IN DATABASE testdb SET work_mem TO '512MB';
SELECT pg_get_database_ddl('testdb');
pg_get_database_ddl
-------------------------------------------------------------------------------------------------------------------
CREATE DATABASE testdb WITH TEMPLATE = template0 ENCODING = 'UTF8'
LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
ALTER DATABASE testdb OWNER = dutow;
ALTER DATABASE testdb SET work_mem TO '256MB';
ALTER DATABASE testdb SET work_mem TO '512MB';

one way is to
compare the output with pg_dump(all). Another point is that these functions can
be used by a dump tool.)

I did my previous testing with this in mind.

Both the role specific database options and the role memberships are
related: I understand if you say that this is a design decision /
limitation. On the other hand if the goal is that users should be able
to replicate dump(all) with these functions, then we should have a
way to also get that information, either by providing two different
outputs, or by specific additional getter functions.

On Fri, Mar 20, 2026, at 10:31 AM, Andrew Dunstan wrote:

Oh, hmm, yes, I think we did. Will work on it.

Here is a new patchset (v2) including all suggested changes in this thread. It
fixes:

* DDLOptType: comma in the last element;
* union for boolval, textval, intval;
* va_list in a restricted scope;
* foreach_ptr + boolean in patches 0002 and 0004;
* list_nth instead of list_nth_cell in patches 0002 and 0004;
* OWNER = role typo. Add test;
* use pstrdup for dbname;
* output only database-specific GUCs;
* add ACL_CONNECT check as the original patch. I removed the pg_read_all_stats
case because it doesn't match the role description;

However, I didn't include the suggestion to explain that pg_get_role_ddl is
dependent on the DateStyle. I think it fits better in the CREATE ROLE [1]https://www.postgresql.org/docs/current/sql-createrole.html that
does not mention it in the VALID UNTIL clause. I'm not opposed to the idea of
adding a sentence to the function description but my suggestion is that this
new sentence points to CREATE ROLE page.

[1]: https://www.postgresql.org/docs/current/sql-createrole.html

--
Euler Taveira
EDB https://www.enterprisedb.com/

v2 is definitely better, I can confirm the improvements.

including all suggested changes in this thread

But I want to point out that the permission check question, and the
role membership question is still open.

For the membership question

I'm not opposed
to your idea but IMO it should be an option.

I'm also fine with leaving it out, but then it should be a mentioned limitation.

For v2:

Shouldn't the tablespace function also support an owner option
similarly to the database function?

A pfree(nulls) and pfree(spcname) seem to be missing, all other
variables are freed properly.

+
+ /*
+ * Variables that are marked GUC_LIST_QUOTE were already fully
+ * quoted before they were put into the setconfig array.  Break
+ * the list value apart and then quote the elements as string
+ * literals.
+ */
+ if (GetConfigOptionFlags(s, true) & GUC_LIST_QUOTE)
+ {

This part seems to be duplicated between the two functions, maybe
could be a helper?

+ALTER ROLE regress_role_ddl_test4 SET search_path TO 'myschema, public';

Maybe it would be useful to also test the "SET search_path TO
myschema, public" variant, without quotes?

I also want to go back to the datestyle question one more time:

The non-fixed DateStyle is by design. It mimics pg_dumpall.

Isn't pg_dump and pg_dumpall inconsistent in this? pg_dump sets it to
ISO, pg_dumpall uses DateStyle. So I'm not that sure about the
"pg_dumpall works this way" argument because pg_dump works
differently. Maybe either of those tools should also be fixed?

The pg_dump behavior is actually a bugfix in cf4cee1b, which was never
applied to pg_dumpall, so it seems like an oversight to me?

On 2026-03-24 Tu 5:56 PM, Zsolt Parragi wrote:

v2 is definitely better, I can confirm the improvements.

including all suggested changes in this thread

But I want to point out that the permission check question, and the
role membership question is still open.

For the membership question

I'm not opposed
to your idea but IMO it should be an option.

I'm also fine with leaving it out, but then it should be a mentioned limitation.

For v2:

Shouldn't the tablespace function also support an owner option
similarly to the database function?

A pfree(nulls) and pfree(spcname) seem to be missing, all other
variables are freed properly.

+
+ /*
+ * Variables that are marked GUC_LIST_QUOTE were already fully
+ * quoted before they were put into the setconfig array.  Break
+ * the list value apart and then quote the elements as string
+ * literals.
+ */
+ if (GetConfigOptionFlags(s, true) & GUC_LIST_QUOTE)
+ {

This part seems to be duplicated between the two functions, maybe
could be a helper?

+ALTER ROLE regress_role_ddl_test4 SET search_path TO 'myschema, public';

Maybe it would be useful to also test the "SET search_path TO
myschema, public" variant, without quotes?

I also want to go back to the datestyle question one more time:

The non-fixed DateStyle is by design. It mimics pg_dumpall.

Isn't pg_dump and pg_dumpall inconsistent in this? pg_dump sets it to
ISO, pg_dumpall uses DateStyle. So I'm not that sure about the
"pg_dumpall works this way" argument because pg_dump works
differently. Maybe either of those tools should also be fixed?

The pg_dump behavior is actually a bugfix in cf4cee1b, which was never
applied to pg_dumpall, so it seems like an oversight to me?

At present, dates are put into a dump in the format specified by the
default datestyle. This is not portable between installations.

This patch sets DATESTYLE to ISO at the start of a pg_dump, so that the
dates written into the dump will be restorable onto any database,
regardless of how its default datestyle is set.

OK, I hope the attached set addresses all the outstanding issues. We're
using ISO dates, and there are appropriate permissions checks. There is
an option to dump role memberships.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

v3 looks good to me

Hi, Andrew

On Tue, 31 Mar 2026 at 15:42, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2026-03-24 Tu 5:56 PM, Zsolt Parragi wrote:

v2 is definitely better, I can confirm the improvements.

including all suggested changes in this thread

But I want to point out that the permission check question, and the
role membership question is still open.

For the membership question

I'm not opposed
to your idea but IMO it should be an option.

I'm also fine with leaving it out, but then it should be a mentioned limitation.

For v2:

Shouldn't the tablespace function also support an owner option
similarly to the database function?

A pfree(nulls) and pfree(spcname) seem to be missing, all other
variables are freed properly.

+
+ /*
+ * Variables that are marked GUC_LIST_QUOTE were already fully
+ * quoted before they were put into the setconfig array.  Break
+ * the list value apart and then quote the elements as string
+ * literals.
+ */
+ if (GetConfigOptionFlags(s, true) & GUC_LIST_QUOTE)
+ {

This part seems to be duplicated between the two functions, maybe
could be a helper?

+ALTER ROLE regress_role_ddl_test4 SET search_path TO 'myschema, public';

Maybe it would be useful to also test the "SET search_path TO
myschema, public" variant, without quotes?

I also want to go back to the datestyle question one more time:

The non-fixed DateStyle is by design. It mimics pg_dumpall.

Isn't pg_dump and pg_dumpall inconsistent in this? pg_dump sets it to
ISO, pg_dumpall uses DateStyle. So I'm not that sure about the
"pg_dumpall works this way" argument because pg_dump works
differently. Maybe either of those tools should also be fixed?

The pg_dump behavior is actually a bugfix in cf4cee1b, which was never
applied to pg_dumpall, so it seems like an oversight to me?

At present, dates are put into a dump in the format specified by the
default datestyle. This is not portable between installations.

This patch sets DATESTYLE to ISO at the start of a pg_dump, so that the
dates written into the dump will be restorable onto any database,
regardless of how its default datestyle is set.

OK, I hope the attached set addresses all the outstanding
issues. We're using ISO dates, and there are appropriate permissions
checks. There is an option to dump role memberships.

Thanks for updating the v3 patches. Here are some comments.

v3-0001
=======

1.
+               List       *namelist;
+               bool            first = true;
+
+               /* Parse string into list of identifiers */
+               if (!SplitGUCList(rawval, ',', &namelist))

According to SplitGUCList()'s comment, the caller should call list_free() on
the returned list even on error. Should we also call list_free() on namelist?

v3-0003
=======

1.
+       /* Add OWNER clause */
+       if (!no_owner)
+       {
+               spcowner = GetUserNameFromId(tspForm->spcowner, false);
+               append_ddl_option(&buf, pretty, 4, "OWNER %s",
+                                                 quote_identifier(spcowner));
+               pfree(spcowner);
+       }

The spcowner is only used within the if scope, so we can narrow its scope.

v3-0004
========

1.
+ append_ddl_option(&buf, pretty, 4, "WITH TEMPLATE = template0");

I'm curious why WITH TEMPLATE = template0 is hardcoded. For example:

[local]:1374846 postgres=# create database db01 IS_TEMPLATE true;
CREATE DATABASE
[local]:1374846 postgres=# create database db02 template db01;
CREATE DATABASE
[local]:1374846 postgres=# select pg_get_database_ddl('db02');
pg_get_database_ddl
-----------------------------------------------------------------------------------------------------------------
CREATE DATABASE db02 WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
ALTER DATABASE db02 OWNER TO japin;
(2 rows)

Is this working as expected?

It seems there's no way to reconstruct the WITH TEMPLATE clause, right?
A comment here would help.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

[2. text/x-patch; v3-0001-Add-infrastructure-for-pg_get_-_ddl-functions.patch]...

[3. text/x-patch; v3-0002-Add-pg_get_role_ddl-function.patch]...

[4. text/x-patch; v3-0003-Add-pg_get_tablespace_ddl-function.patch]...

[5. text/x-patch; v3-0004-Add-pg_get_database_ddl-function.patch]...

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

On Thursday, April 2, 2026, Japin Li <japinli@hotmail.com> wrote:

v3-0004
========

1.
+ append_ddl_option(&buf, pretty, 4, "WITH TEMPLATE = template0");

I'm curious why WITH TEMPLATE = template0 is hardcoded. For example:

[local]:1374846 postgres=# create database db01 IS_TEMPLATE true;
CREATE DATABASE
[local]:1374846 postgres=# create database db02 template db01;
CREATE DATABASE
[local]:1374846 postgres=# select pg_get_database_ddl('db02');
pg_get_database_ddl
------------------------------------------------------------
-----------------------------------------------------
CREATE DATABASE db02 WITH TEMPLATE = template0 ENCODING = 'UTF8'
LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
ALTER DATABASE db02 OWNER TO japin;
(2 rows)

Is this working as expected?

It seems there's no way to reconstruct the WITH TEMPLATE clause, right?
A comment here would help.

There is no way or use in constructing the original template clause, though
I agree it’s worth a comment. At the end of the day the catalog data that
was found in the db01 database already exists in the db02 database when
executing these DLL reconstruction functions against the existing db02
database. Taking nothing from the template is the correct behavior - hence
template0.

David J.

On 2026-04-02 Th 9:35 AM, David G. Johnston wrote:

On Thursday, April 2, 2026, Japin Li <japinli@hotmail.com> wrote:

v3-0004
========

1.
+       append_ddl_option(&buf, pretty, 4, "WITH TEMPLATE =
template0");

I'm curious why WITH TEMPLATE = template0 is hardcoded. For example:

  [local]:1374846 postgres=# create database db01 IS_TEMPLATE true;
  CREATE DATABASE
  [local]:1374846 postgres=# create database db02 template db01;
  CREATE DATABASE
  [local]:1374846 postgres=# select pg_get_database_ddl('db02');
 pg_get_database_ddl
 
-----------------------------------------------------------------------------------------------------------------
   CREATE DATABASE db02 WITH TEMPLATE = template0 ENCODING =
'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
   ALTER DATABASE db02 OWNER TO japin;
  (2 rows)

Is this working as expected?

It seems there's no way to reconstruct the WITH TEMPLATE clause,
right?
A comment here would help.

There is no way or use in constructing the original template clause,
though I agree it’s worth a comment.  At the end of the day the
catalog data that was found in the db01 database already exists in the
db02 database when executing these DLL reconstruction functions
against the existing db02 database. Taking nothing from the template
is the correct behavior - hence template0.

OK, here's a v4.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On 2026-04-02 Th 12:27 PM, Andrew Dunstan wrote:

On 2026-04-02 Th 9:35 AM, David G. Johnston wrote:

On Thursday, April 2, 2026, Japin Li <japinli@hotmail.com> wrote:

v3-0004
========

1.
+       append_ddl_option(&buf, pretty, 4, "WITH TEMPLATE =
template0");

I'm curious why WITH TEMPLATE = template0 is hardcoded. For example:

  [local]:1374846 postgres=# create database db01 IS_TEMPLATE true;
  CREATE DATABASE
  [local]:1374846 postgres=# create database db02 template db01;
  CREATE DATABASE
  [local]:1374846 postgres=# select pg_get_database_ddl('db02');
 pg_get_database_ddl
 
-----------------------------------------------------------------------------------------------------------------
   CREATE DATABASE db02 WITH TEMPLATE = template0 ENCODING =
'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
   ALTER DATABASE db02 OWNER TO japin;
  (2 rows)

Is this working as expected?

It seems there's no way to reconstruct the WITH TEMPLATE clause,
right?
A comment here would help.

There is no way or use in constructing the original template clause,
though I agree it’s worth a comment.  At the end of the day the
catalog data that was found in the db01 database already exists in
the db02 database when executing these DLL reconstruction functions
against the existing db02 database.  Taking nothing from the template
is the correct behavior - hence template0.

OK, here's a v4.

Pushed. I have moved the remaining get_*_ddl items to PG20-1

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Hi,

On 2026-04-05 11:06:09 -0400, Andrew Dunstan wrote:

Pushed. I have moved the remaining get_*_ddl items to PG20-1

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&amp;dt=2026-04-05%2015%3A04%3A04

diff -U3 /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/database_ddl.out /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/results/database_ddl.out
--- /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/database_ddl.out	2026-04-05 11:04:08
+++ /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/results/database_ddl.out	2026-04-05 11:05:57
@@ -22,6 +22,7 @@
 CREATE DATABASE regress_database_ddl
     ENCODING utf8 LC_COLLATE "C" LC_CTYPE "C" TEMPLATE template0
     OWNER regress_datdba;
+WARNING:  databases created by regression test cases should have names including "regression"
 ALTER DATABASE regress_database_ddl CONNECTION_LIMIT 123;
 ALTER DATABASE regress_database_ddl SET random_page_cost = 2.0;
 ALTER ROLE regress_datdba IN DATABASE regress_database_ddl SET random_page_cost = 1.1;

But do we really have to create a new database and a new tablespace for these?
Database and tablespace creations are quite heavyweight operations.

We already have an existing tablespace and an existing database as part of the
regression tests. Couldn't you make do with those?

Greetings,

Anres