Обсуждение: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement

On 11/12/25 8:04 PM, Akshay Joshi wrote:
> Hi Hackers,
> 
> I’m submitting a patch as part of the broader Retail DDL Functions 
> project described by Andrew Dunstan https://www.postgresql.org/message- 
> id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net <https:// 
> www.postgresql.org/message-id/945db7c5-be75-45bf-b55b- 
> cb1e56f2e3e9%40dunslane.net>
> 
> This patch adds a new system function pg_get_database_ddl(database_name/ 
> database_oid, pretty), which reconstructs the CREATE DATABASE statement 
> for a given database name or database oid. When the pretty flag is set 
> to true, the function returns a neatly formatted, multi-line DDL 
> statement instead of a single-line statement.
> 
> *Usage examples:*
> 
> 1) SELECT pg_get_database_ddl('test_get_database_ddl_builtin');  -- 
> *non-pretty formatted DDL*
>                                                                          
>                                                              
> pg_get_database_ddl
>
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>    CREATE DATABASE test_get_database_ddl_builtin WITH OWNER = 
> regress_ddl_database ENCODING = "UTF8" LC_COLLATE = "C" LC_CTYPE = "C" 
> BUILTIN_LOCALE = "C.UTF-8" COLLATION_VERSION = "1" LOCALE_PROVIDER = 
> 'builtin' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION 
> LIMIT = -1;
> 
> 
> 2) SELECT pg_get_database_ddl('test_get_database_ddl_builtin', true);   
> -- *pretty formatted DDL*
> 
> CREATE DATABASE test_get_database_ddl_builtin
>           WITH
>           OWNER = regress_ddl_database
>           ENCODING = "UTF8"
>           LC_COLLATE = "C"
>           LC_CTYPE = "C"
>           BUILTIN_LOCALE = "C.UTF-8"
>           COLLATION_VERSION = "1"
>           LOCALE_PROVIDER = 'builtin'
>           TABLESPACE = pg_default
>           ALLOW_CONNECTIONS = true
>           CONNECTION LIMIT = -1;
> 
> 3) SELECT pg_get_database_ddl(16835);      -- *non-pretty formatted DDL 
> for OID*
> 4) SELECT pg_get_database_ddl(16835, true);  -- *pretty formatted DDL 
> for OID*
> 
> The patch includes documentation, in-code comments, and regression 
> tests, all of which pass successfully.
> *
> **Note:* To run the regression tests, particularly the pg_upgrade tests 
> successfully, I had to add a helper function, ddl_filter (in 
> database.sql), which removes locale and collation-related information 
> from the pg_get_database_ddl output.
> 
I think we should check the connection permissions here. Otherwise:

postgres=> SELECT pg_database_size('testdb');
ERROR:  permission denied for database testdb
postgres=> SELECT pg_get_database_ddl('testdb');
  
                         pg_get_database_ddl

-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  CREATE DATABASE testdb WITH OWNER = quanzl ENCODING = "UTF8" 
LC_COLLATE = "zh_CN.UTF-8" LC_CTYPE = "zh_CN.UTF-8" LOCALE_PROVIDER = 
'libc' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION LIMIT 
= -1;
(1 row)

Users without connection permissions should not generate DDL.

Regards,
Quan Zongliang

> -----
> Regards,
> Akshay Joshi
> EDB (EnterpriseDB)
> 
> 
> 

On 11/13/25 12:17 PM, Quan Zongliang wrote:
> 
> 
> On 11/12/25 8:04 PM, Akshay Joshi wrote:
>> Hi Hackers,
>>
>> I’m submitting a patch as part of the broader Retail DDL Functions 
>> project described by Andrew Dunstan https://www.postgresql.org/ 
>> message- id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net 
>> <https:// www.postgresql.org/message-id/945db7c5-be75-45bf-b55b- 
>> cb1e56f2e3e9%40dunslane.net>
>>
>> This patch adds a new system function 
>> pg_get_database_ddl(database_name/ database_oid, pretty), which 
>> reconstructs the CREATE DATABASE statement for a given database name 
>> or database oid. When the pretty flag is set to true, the function 
>> returns a neatly formatted, multi-line DDL statement instead of a 
>> single-line statement.
>>
>> *Usage examples:*
>>
>> 1) SELECT pg_get_database_ddl('test_get_database_ddl_builtin');  -- 
>> *non-pretty formatted DDL*
>> pg_get_database_ddl
>>
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>    CREATE DATABASE test_get_database_ddl_builtin WITH OWNER = 
>> regress_ddl_database ENCODING = "UTF8" LC_COLLATE = "C" LC_CTYPE = "C" 
>> BUILTIN_LOCALE = "C.UTF-8" COLLATION_VERSION = "1" LOCALE_PROVIDER = 
>> 'builtin' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION 
>> LIMIT = -1;
>>
>>
>> 2) SELECT pg_get_database_ddl('test_get_database_ddl_builtin', true); 
>> -- *pretty formatted DDL*
>>
>> CREATE DATABASE test_get_database_ddl_builtin
>>           WITH
>>           OWNER = regress_ddl_database
>>           ENCODING = "UTF8"
>>           LC_COLLATE = "C"
>>           LC_CTYPE = "C"
>>           BUILTIN_LOCALE = "C.UTF-8"
>>           COLLATION_VERSION = "1"
>>           LOCALE_PROVIDER = 'builtin'
>>           TABLESPACE = pg_default
>>           ALLOW_CONNECTIONS = true
>>           CONNECTION LIMIT = -1;
>>
>> 3) SELECT pg_get_database_ddl(16835);      -- *non-pretty formatted 
>> DDL for OID*
>> 4) SELECT pg_get_database_ddl(16835, true);  -- *pretty formatted DDL 
>> for OID*
>>
>> The patch includes documentation, in-code comments, and regression 
>> tests, all of which pass successfully.
>> *
>> **Note:* To run the regression tests, particularly the pg_upgrade 
>> tests successfully, I had to add a helper function, ddl_filter (in 
>> database.sql), which removes locale and collation-related information 
>> from the pg_get_database_ddl output.
>>
> I think we should check the connection permissions here. Otherwise:
> 
> postgres=> SELECT pg_database_size('testdb');
> ERROR:  permission denied for database testdb
> postgres=> SELECT pg_get_database_ddl('testdb');
> 
>                          pg_get_database_ddl
>
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>   CREATE DATABASE testdb WITH OWNER = quanzl ENCODING = "UTF8" 
> LC_COLLATE = "zh_CN.UTF-8" LC_CTYPE = "zh_CN.UTF-8" LOCALE_PROVIDER = 
> 'libc' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION LIMIT 
> = -1;
> (1 row)
> 
> Users without connection permissions should not generate DDL.
> 

The "dbOwner" is defined as a null pointer.
char       *dbOwner = NULL;

Later, there might be a risk of it not being assigned a value.
    if (OidIsValid(dbForm->datdba))
       dbOwner = GetUserNameFromId(dbForm->datdba, false);

Although there is no problem in normal circumstances here. Many parts of 
the existing code have not been checked either. Since this possibility 
exists, it should be checked before using it. Just like the function 
roles_is_member_of (acl.c).

if (dbOwner)
   get_formatted_string(&buf, prettyFlags, 1, "OWNER = %s",
             quote_identifier(dbOwner));

> Regards,
> Quan Zongliang
> 
>> -----
>> Regards,
>> Akshay Joshi
>> EDB (EnterpriseDB)
>>
>>
>>
> 
> 

On 11/13/25 4:30 PM, Akshay Joshi wrote:
> 
> On Thu, Nov 13, 2025 at 9:47 AM Quan Zongliang <quanzongliang@yeah.net 
> <mailto:quanzongliang@yeah.net>> wrote:
> 
> 
> 
>     On 11/12/25 8:04 PM, Akshay Joshi wrote:
>      > Hi Hackers,
>      >
>      > I’m submitting a patch as part of the broader Retail DDL Functions
>      > project described by Andrew Dunstan https://www.postgresql.org/
>     message- <https://www.postgresql.org/message->
>      > id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net
>     <http://40dunslane.net> <https://
>      > www.postgresql.org/message-id/945db7c5-be75-45bf-b55b- <http://
>     www.postgresql.org/message-id/945db7c5-be75-45bf-b55b->
>      > cb1e56f2e3e9%40dunslane.net <http://40dunslane.net>>
>      >
>      > This patch adds a new system function
>     pg_get_database_ddl(database_name/
>      > database_oid, pretty), which reconstructs the CREATE DATABASE
>     statement
>      > for a given database name or database oid. When the pretty flag
>     is set
>      > to true, the function returns a neatly formatted, multi-line DDL
>      > statement instead of a single-line statement.
>      >
>      > *Usage examples:*
>      >
>      > 1) SELECT pg_get_database_ddl('test_get_database_ddl_builtin');  --
>      > *non-pretty formatted DDL*
>      >
>      >
>      > pg_get_database_ddl
>      >
>
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>      >    CREATE DATABASE test_get_database_ddl_builtin WITH OWNER =
>      > regress_ddl_database ENCODING = "UTF8" LC_COLLATE = "C" LC_CTYPE
>     = "C"
>      > BUILTIN_LOCALE = "C.UTF-8" COLLATION_VERSION = "1" LOCALE_PROVIDER =
>      > 'builtin' TABLESPACE = pg_default ALLOW_CONNECTIONS = true
>     CONNECTION
>      > LIMIT = -1;
>      >
>      >
>      > 2) SELECT pg_get_database_ddl('test_get_database_ddl_builtin',
>     true);
>      > -- *pretty formatted DDL*
>      >
>      > CREATE DATABASE test_get_database_ddl_builtin
>      >           WITH
>      >           OWNER = regress_ddl_database
>      >           ENCODING = "UTF8"
>      >           LC_COLLATE = "C"
>      >           LC_CTYPE = "C"
>      >           BUILTIN_LOCALE = "C.UTF-8"
>      >           COLLATION_VERSION = "1"
>      >           LOCALE_PROVIDER = 'builtin'
>      >           TABLESPACE = pg_default
>      >           ALLOW_CONNECTIONS = true
>      >           CONNECTION LIMIT = -1;
>      >
>      > 3) SELECT pg_get_database_ddl(16835);      -- *non-pretty
>     formatted DDL
>      > for OID*
>      > 4) SELECT pg_get_database_ddl(16835, true);  -- *pretty formatted
>     DDL
>      > for OID*
>      >
>      > The patch includes documentation, in-code comments, and regression
>      > tests, all of which pass successfully.
>      > *
>      > **Note:* To run the regression tests, particularly the pg_upgrade
>     tests
>      > successfully, I had to add a helper function, ddl_filter (in
>      > database.sql), which removes locale and collation-related
>     information
>      > from the pg_get_database_ddl output.
>      >
>     I think we should check the connection permissions here. Otherwise:
> 
>     postgres=> SELECT pg_database_size('testdb');
>     ERROR:  permission denied for database testdb
>     postgres=> SELECT pg_get_database_ddl('testdb');
> 
>                               pg_get_database_ddl
>
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>        CREATE DATABASE testdb WITH OWNER = quanzl ENCODING = "UTF8"
>     LC_COLLATE = "zh_CN.UTF-8" LC_CTYPE = "zh_CN.UTF-8" LOCALE_PROVIDER =
>     'libc' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION
>     LIMIT
>     = -1;
>     (1 row)
> 
>     Users without connection permissions should not generate DDL.
> 
> 
> pg_database_size() requires CONNECT or pg_read_all_stats privileges 
> since it accesses on-disk storage details of a database, which are 
> treated as sensitive information. In contrast, other system functions 
> might not need such privileges because they operate within the connected 
> database or reveal less sensitive data.
> 
> In my view, the pg_get_database_ddl() function *should not* require 
> CONNECT or pg_read_all_stats privileges for consistency and security.
>
Agree.

But what about the following scenario? If there is no permission to 
access pg_database. Shouldn't the DDL be returned?

postgres=> SELECT * FROM pg_database;
ERROR:  permission denied for table pg_database
postgres=> SELECT pg_get_database_ddl('testdb');

> 
>     Regards,
>     Quan Zongliang
> 
>      > -----
>      > Regards,
>      > Akshay Joshi
>      > EDB (EnterpriseDB)
>      >
>      >
>      >
> 

On 11/13/25 8:28 PM, Álvaro Herrera wrote:

>> But what about the following scenario? If there is no permission to access
>> pg_database. Shouldn't the DDL be returned?
>>
>> postgres=> SELECT * FROM pg_database;
>> ERROR:  permission denied for table pg_database
>> postgres=> SELECT pg_get_database_ddl('testdb');
> 
> Hmm, what scenario is this?  Did you purposefully REVOKE the SELECT
> privileges from pg_database somehow?
> 
Yes. I revoked the access permission using the REVOKE command.

The pg_get_xxx_ddl function is actually revealing system information to 
the users. This is equivalent to accessing the corresponding system 
table. So I think we should consider this issue.

The access permission to the system tables has been revoked. This user 
is unable to directly view the contents of the system tables from the 
client side via SQL. However, it is still possible to obtain the object 
definitions (which was previously inaccessible) through pg_get_xxx_ddl. 
This is more like a security flaw.

A more specific example. Originally, it was impossible to obtain the 
definition of "testdb" by accessing pg_database:

   postgres=> SELECT * FROM pg_database WHERE datname='testdb';
   ERROR:  permission denied for table pg_database

And after having this function. However, users can view these in another 
format.

   postgres=> SELECT pg_get_database_ddl('testdb');
   ------------- ...
   CREATE DATABASE testdb WITH OWNER = quanzl ENCODING = "UTF8" ...

Perhaps it's just that I'm overthinking things. What do you think about it?

On Thu, Nov 13, 2025 at 02:02:30PM +0530, Akshay Joshi wrote:
> On Thu, Nov 13, 2025 at 10:18 AM Quan Zongliang <quanzongliang@yeah.net>
> wrote:
> 
> >
> >
> > On 11/13/25 12:17 PM, Quan Zongliang wrote:
> > >
> > >
> > > On 11/12/25 8:04 PM, Akshay Joshi wrote:
> > >> Hi Hackers,
> > >>
> > >> I’m submitting a patch as part of the broader Retail DDL Functions
> > >> project described by Andrew Dunstan https://www.postgresql.org/
> > >> message- id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9%40dunslane.net
> > >> <https:// www.postgresql.org/message-id/945db7c5-be75-45bf-b55b-
> > >> cb1e56f2e3e9%40dunslane.net>
> > >>
> > >> This patch adds a new system function
> > >> pg_get_database_ddl(database_name/ database_oid, pretty), which
> > >> reconstructs the CREATE DATABASE statement for a given database name
> > >> or database oid. When the pretty flag is set to true, the function
> > >> returns a neatly formatted, multi-line DDL statement instead of a
> > >> single-line statement.
> > >>
> > >> *Usage examples:*
> > >>
> > >> 1) SELECT pg_get_database_ddl('test_get_database_ddl_builtin');  --
> > >> *non-pretty formatted DDL*
> > >> pg_get_database_ddl
> > >>
> >
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> > >>    CREATE DATABASE test_get_database_ddl_builtin WITH OWNER =
> > >> regress_ddl_database ENCODING = "UTF8" LC_COLLATE = "C" LC_CTYPE = "C"
> > >> BUILTIN_LOCALE = "C.UTF-8" COLLATION_VERSION = "1" LOCALE_PROVIDER =
> > >> 'builtin' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION
> > >> LIMIT = -1;
> > >>
> > >>
> > >> 2) SELECT pg_get_database_ddl('test_get_database_ddl_builtin', true);
> > >> -- *pretty formatted DDL*
> > >>
> > >> CREATE DATABASE test_get_database_ddl_builtin
> > >>           WITH
> > >>           OWNER = regress_ddl_database
> > >>           ENCODING = "UTF8"
> > >>           LC_COLLATE = "C"
> > >>           LC_CTYPE = "C"
> > >>           BUILTIN_LOCALE = "C.UTF-8"
> > >>           COLLATION_VERSION = "1"
> > >>           LOCALE_PROVIDER = 'builtin'
> > >>           TABLESPACE = pg_default
> > >>           ALLOW_CONNECTIONS = true
> > >>           CONNECTION LIMIT = -1;
> > >>
> > >> 3) SELECT pg_get_database_ddl(16835);      -- *non-pretty formatted
> > >> DDL for OID*
> > >> 4) SELECT pg_get_database_ddl(16835, true);  -- *pretty formatted DDL
> > >> for OID*
> > >>
> > >> The patch includes documentation, in-code comments, and regression
> > >> tests, all of which pass successfully.
> > >> *
> > >> **Note:* To run the regression tests, particularly the pg_upgrade
> > >> tests successfully, I had to add a helper function, ddl_filter (in
> > >> database.sql), which removes locale and collation-related information
> > >> from the pg_get_database_ddl output.
> > >>
> > > I think we should check the connection permissions here. Otherwise:
> > >
> > > postgres=> SELECT pg_database_size('testdb');
> > > ERROR:  permission denied for database testdb
> > > postgres=> SELECT pg_get_database_ddl('testdb');
> > >
> > >                          pg_get_database_ddl
> > >
> >
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> > >   CREATE DATABASE testdb WITH OWNER = quanzl ENCODING = "UTF8"
> > > LC_COLLATE = "zh_CN.UTF-8" LC_CTYPE = "zh_CN.UTF-8" LOCALE_PROVIDER =
> > > 'libc' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION LIMIT
> > > = -1;
> > > (1 row)
> > >
> > > Users without connection permissions should not generate DDL.
> > >
> >
> > The "dbOwner" is defined as a null pointer.
> > char       *dbOwner = NULL;
> >
> > Later, there might be a risk of it not being assigned a value.
> >     if (OidIsValid(dbForm->datdba))
> >        dbOwner = GetUserNameFromId(dbForm->datdba, false);
> >
> > Although there is no problem in normal circumstances here. Many parts of
> > the existing code have not been checked either. Since this possibility
> > exists, it should be checked before using it. Just like the function
> > roles_is_member_of (acl.c).
> >
> > if (dbOwner)
> >    get_formatted_string(&buf, prettyFlags, 1, "OWNER = %s",
> >                          quote_identifier(dbOwner));
> >
> 
>  Fixed the given review comment. I've attached the v2 patch ready for
> review.
> 

Thanks for updating the patch, some comments on v2.

1.
Should we merge the functions pg_get_database_ddl(oid, [boolean]) and
pg_get_database_ddl(name, [boolean]) in documentation, following the
precedent set by pg_database_size in func-admin.sgml.

2.
+ * noOfTabChars - indent with specified no of tabs.

How about using 'indent with specified number of tab characters'?
And for variable noOfTabChars, I'd like use nTabs or nTabChars.

3.
+/*
+ * pg_get_database_ddl_oid
+ *
+ * Generate a CREATE DATABASE statement for the specified database oid.
+ *
+ * dbName - Name of the database for which to generate the DDL.
+ * pretty - If true, format the DDL with indentation and line breaks.
+ */

A copy-paste error resulted in an incorrect comments (dbName).

-- 
Best regards,
Japin Li
ChengDu WenWu Information Technology Co., LTD.

Hi Akshay,

I quick went through the patch, I do see some problems, but I need some time to wrap up, so I will do a deep review
nextweek. In the meantime, I want to first ask that why there is no privilege check? I think that’s a serious issue. 

> On Nov 13, 2025, at 16:32, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
>
>
> <v2-0001-Add-pg_get_database_ddl-function-to-reconstruct-CREATE.patch>

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

On 2025-Nov-13, Quan Zongliang wrote:

> A more specific example. Originally, it was impossible to obtain the
> definition of "testdb" by accessing pg_database:
> 
>   postgres=> SELECT * FROM pg_database WHERE datname='testdb';
>   ERROR:  permission denied for table pg_database

Hmm.  So I was thinking that running things in this mode (where catalog
access is restricted) has never been supported.  But you're right that
we would be opening a hole that we don't have today, because if the
admin closes down permissions on pg_database, then this new function
would be a way to obtain information that the user can't currently
obtain.

My further point was to be that you still need to obtain a list of
database names or OIDs in order to do anything of value.  But it turns
out that this is extremely easy and quick to do, with something like

SELECT i, pg_describe_object('pg_database'::regclass, i, 0)
FROM generate_series(1, 1_000_000) i
WHERE pg_describe_object('pg_database'::regclass, i, 0) IS NOT NULL;

... and with this function, the user could again obtain everything about
the database even when they can't read the catalog directly.

Maybe checking privs for the database being dumped is enough protection
against this -- the equivalent of has_database_privilege( ..., 'CONNECT') 
I suppose.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"¿Qué importan los años?  Lo que realmente importa es comprobar que
a fin de cuentas la mejor edad de la vida es estar vivo"  (Mafalda)

Hi Akshay,

I just reviewed v3 and got some comments:

> On Nov 17, 2025, at 22:34, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
>
> All the review comments have been addressed in v3 patch.


1 - ruleutils.c
```
+    if (dbForm->datconnlimit != 0)
+        get_formatted_string(&buf, prettyFlags, 1, "CONNECTION LIMIT = %d",
+                             dbForm->datconnlimit);
```

I think this is wrong. Default value of CONNECTION_LIMIT is -1 rather than 0. 0 means no connection is allowed, users
shouldintentionally set the value, thus 0 should be printed. 

2 - ruleutils.c
```
+    if (!attrIsNull)
+        get_formatted_string(&buf, prettyFlags, 1, "ICU_RULES = %s",
+                             quote_identifier(TextDatumGetCString(dbValue)));
```

ICU_RULES should be omitted if provider is not icu.

3 - ruleutils.c
```
+    if (!HeapTupleIsValid(tupleDatabase))
+        ereport(ERROR,
+                errcode(ERRCODE_UNDEFINED_OBJECT),
+                errmsg("database with oid %d does not exist", dbOid));
```

I believe all existing code use %u to format oid. I ever raised the same comment to the other get_xxx_ddl patch.

4 - ruleutils.c
```
+    /*
+     * User must have connect privilege for target database.
+     */
+    aclresult = object_aclcheck(DatabaseRelationId, dbOid, GetUserId(),
+                                ACL_CONNECT);
+    if (aclresult != ACLCHECK_OK)
+    {
+        aclcheck_error(aclresult, OBJECT_DATABASE,
+                       get_database_name(dbOid));
+    }
```

I don’t think CONNECT privilege is good enough. By default, a new user gets CONNECT privilege via the PUBLIC role. I
justdid a quick test to confirm that. 

```
# Create a new cluster
% initdb .
% pg_ctl -D . start
% createdb evantest
% createdb evan

# connect to the db
% psql -d evantest -U evan
psql (19devel)
Type "help" for help. # Got into the database successfully

# Without any privilege grant, the user can get ddl of the system database, which seems not good
evantest=> select pg_get_database_ddl('postgres', true);
        pg_get_database_ddl
------------------------------------
 CREATE DATABASE postgres          +
         WITH                      +
         OWNER = chaol             +
         ENCODING = "UTF8"         +
         LC_COLLATE = "en_US.UTF-8"+
         LC_CTYPE = "en_US.UTF-8"  +
         LOCALE_PROVIDER = 'libc'  +
         TABLESPACE = pg_default   +
         ALLOW_CONNECTIONS = true  +
         CONNECTION LIMIT = -1;
(1 row)
```

IMO, only super user and database owner should be to get ddl of the database.

5 - as you can see from the above test output, “+” in the end of every line is weird.

6 - “WITH” has the same indent as the parameters, which doesn’t good look. If we look at the doc
https://www.postgresql.org/docs/18/sql-createdatabase.html,“WITH” takes the first level of indent, and parameters take
thesecond level of indent. 

7 - For those parameters that have default values should be omitted. For example:
```
evantest=> select pg_get_database_ddl('evantest', true);
        pg_get_database_ddl
------------------------------------
 CREATE DATABASE evantest          +
         WITH                      +
         OWNER = chaol             +
         ENCODING = "UTF8"         +
         LC_COLLATE = "en_US.UTF-8"+
         LC_CTYPE = "en_US.UTF-8"  +
         LOCALE_PROVIDER = 'libc'  +
         TABLESPACE = pg_default   +
         ALLOW_CONNECTIONS = true  +
         CONNECTION LIMIT = -1;
(1 row)
```

I created the database “evantest” without providing any parameter. I think at least OWNER, TABLESPACE and
ALLOW_CNONNECTIONSshould be omitted. For CONNECTION LIMIT, you already have a logic to omit it but the logic has some
problemas comment 1. 

8 - ruleutils.c
```
+/*
+ * get_formatted_string
+ *
+ * Return a formatted version of the string.
+ *
+ * prettyFlags - Based on prettyFlags the output includes tabs (\t) and
+ *               newlines (\n).
+ * nTabChars - indent with specified number of tab characters.
+ * fmt - printf-style format string used by appendStringInfoVA.
+ */
+static void
+get_formatted_string(StringInfo buf, int prettyFlags, int nTabChars, const char *fmt,...)
```

I don’t feel good with this function, but not because of the implementation of the function.

I have reviewed a bunch of get_xxx_ddl patches submitted by different persons. All of them are under a big project,
however,looks like to me that all authors work independently without properly coordinated. A function like this one
shouldbe common for all those patches. Maybe Alvaro can help here, pushing a common function that is used to format DDL
andrequesting all patches to use the function. 

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

Hi Akshay,

Thanks for updating the patch.

On Tue, 18 Nov 2025 at 13:33, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
> Hi Chao
>
> Thanks for reviewing my patch.
>
> On Tue, Nov 18, 2025 at 5:59 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>  Hi Akshay,
>
>  I just reviewed v3 and got some comments:
>
>  > On Nov 17, 2025, at 22:34, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
>  >
>  > All the review comments have been addressed in v3 patch.
>
>  1 - ruleutils.c
>  ```
>  +       if (dbForm->datconnlimit != 0)
>  +               get_formatted_string(&buf, prettyFlags, 1, "CONNECTION LIMIT = %d",
>  +                                                        dbForm->datconnlimit);
>  ```
>
>  I think this is wrong. Default value of CONNECTION_LIMIT is -1 rather than 0. 0 means no connection is allowed,
users
>  should intentionally set the value, thus 0 should be printed.
>
>  2 - ruleutils.c
>  ```
>  +       if (!attrIsNull)
>  +               get_formatted_string(&buf, prettyFlags, 1, "ICU_RULES = %s",
>  +                                                        quote_identifier(TextDatumGetCString(dbValue)));
>  ```
>
>  ICU_RULES should be omitted if provider is not icu.
>
>  3 - ruleutils.c
>  ```
>  +       if (!HeapTupleIsValid(tupleDatabase))
>  +               ereport(ERROR,
>  +                               errcode(ERRCODE_UNDEFINED_OBJECT),
>  +                               errmsg("database with oid %d does not exist", dbOid));
>  ```
>
>  I believe all existing code use %u to format oid. I ever raised the same comment to the other get_xxx_ddl patch.
>
>  Fixed all above in the attached v4 patch.

1.
Since the STRATEGY and OID parameters are not being reconstructed, should we
document this behavior?

postgres=# CREATE DATABASE mydb WITH STRATEGY file_copy OID 19876 IS_TEMPLATE true;
CREATE DATABASE
postgres=# SELECT pg_get_database_ddl('mydb', true);
            pg_get_database_ddl
--------------------------------------------
 CREATE DATABASE mydb                      +
         WITH OWNER = japin                +
                 ENCODING = "UTF8"         +
                 LC_COLLATE = "en_US.UTF-8"+
                 LC_CTYPE = "en_US.UTF-8"  +
                 COLLATION_VERSION = "2.39"+
                 LOCALE_PROVIDER = 'libc'  +
                 TABLESPACE = pg_default   +
                 ALLOW_CONNECTIONS = true  +
                 CONNECTION LIMIT = -1     +
                 IS_TEMPLATE = true;
(1 row)

2.
We can restrict the scope of the dbTablespace variable as follows:

+    if (OidIsValid(dbForm->dattablespace))
+    {
+        char *dbTablespace = get_tablespace_name(dbForm->dattablespace);
+        if (dbTablespace)
+            get_formatted_string(&buf, prettyFlags, 2, "TABLESPACE = %s",
+                                 quote_identifier(dbTablespace));
+    }

> >
> > 7 - For those parameters that have default values should be omitted. For
> > example:
> > ```
> > evantest=> select pg_get_database_ddl('evantest', true);
> >         pg_get_database_ddl
> > ------------------------------------
> >  CREATE DATABASE evantest          +
> >          WITH                      +
> >          OWNER = chaol             +
> >          ENCODING = "UTF8"         +
> >          LC_COLLATE = "en_US.UTF-8"+
> >          LC_CTYPE = "en_US.UTF-8"  +
> >          LOCALE_PROVIDER = 'libc'  +
> >          TABLESPACE = pg_default   +
> >          ALLOW_CONNECTIONS = true  +
> >          CONNECTION LIMIT = -1;
> > (1 row)
> > ```
> >
> > I created the database “evantest” without providing any parameter. I think
> > at least OWNER, TABLESPACE and ALLOW_CNONNECTIONS should be omitted. For
> > CONNECTION LIMIT, you already have a logic to omit it but the logic has
> > some problem as comment 1.
> >
>
>
>
> IMHO, parameters with default values *should not* be omitted from the
> output of the pg_get_xxx_ddl functions. The primary purpose of these
> functions is to accurately reconstruct the DDL. Including all parameters
> ensures clarity, as not everyone is familiar with the default value of
> every single parameter.

+1

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

Hello,

One thing I realized a few days ago is that since commit bd09f024a1bb we
have type regdatabase, so instead of having two functions (one taking
name and one taking Oid), we should have just one, taking regdatabase,
just like the functions for producing DDL for other object types that
have corresponding reg* type.

Regards,

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Wed, 19 Nov 2025 at 16:36, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
> On Wed, Nov 19, 2025 at 3:48 PM Japin Li <japinli@hotmail.com> wrote:
>
>  Hi Akshay,
>
>  Thanks for updating the patch.
>
>  On Tue, 18 Nov 2025 at 13:33, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
>  > Hi Chao
>  >
>  > Thanks for reviewing my patch.
>  >
>  > On Tue, Nov 18, 2025 at 5:59 AM Chao Li <li.evan.chao@gmail.com> wrote:
>  >
>  >  Hi Akshay,
>  >
>  >  I just reviewed v3 and got some comments:
>  >
>  >  > On Nov 17, 2025, at 22:34, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
>  >  >
>  >  > All the review comments have been addressed in v3 patch.
>  >
>  >  1 - ruleutils.c
>  >  ```
>  >  +       if (dbForm->datconnlimit != 0)
>  >  +               get_formatted_string(&buf, prettyFlags, 1, "CONNECTION LIMIT = %d",
>  >  +                                                        dbForm->datconnlimit);
>  >  ```
>  >
>  >  I think this is wrong. Default value of CONNECTION_LIMIT is -1 rather than 0. 0 means no connection is allowed,
>  users
>  >  should intentionally set the value, thus 0 should be printed.
>  >
>  >  2 - ruleutils.c
>  >  ```
>  >  +       if (!attrIsNull)
>  >  +               get_formatted_string(&buf, prettyFlags, 1, "ICU_RULES = %s",
>  >  +                                                        quote_identifier(TextDatumGetCString(dbValue)));
>  >  ```
>  >
>  >  ICU_RULES should be omitted if provider is not icu.
>  >
>  >  3 - ruleutils.c
>  >  ```
>  >  +       if (!HeapTupleIsValid(tupleDatabase))
>  >  +               ereport(ERROR,
>  >  +                               errcode(ERRCODE_UNDEFINED_OBJECT),
>  >  +                               errmsg("database with oid %d does not exist", dbOid));
>  >  ```
>  >
>  >  I believe all existing code use %u to format oid. I ever raised the same comment to the other get_xxx_ddl patch.
>  >
>  >  Fixed all above in the attached v4 patch.
>
>  1.
>  Since the STRATEGY and OID parameters are not being reconstructed, should we
>  document this behavior?
>
>  postgres=# CREATE DATABASE mydb WITH STRATEGY file_copy OID 19876 IS_TEMPLATE true;
>  CREATE DATABASE
>  postgres=# SELECT pg_get_database_ddl('mydb', true);
>              pg_get_database_ddl
>  --------------------------------------------
>   CREATE DATABASE mydb                      +
>           WITH OWNER = japin                +
>                   ENCODING = "UTF8"         +
>                   LC_COLLATE = "en_US.UTF-8"+
>                   LC_CTYPE = "en_US.UTF-8"  +
>                   COLLATION_VERSION = "2.39"+
>                   LOCALE_PROVIDER = 'libc'  +
>                   TABLESPACE = pg_default   +
>                   ALLOW_CONNECTIONS = true  +
>                   CONNECTION LIMIT = -1     +
>                   IS_TEMPLATE = true;
>  (1 row)
>
> The FormData_pg_database structure does not expose the database STRATEGY,

Yes, it's not exposed.

> and reconstructing the OID serves no practical
> purpose. If the majority agrees, I can extend the DDL to include OID.

I'm not insisting on reconstructing those parameters; I mean, we can provide
a sentence to describe this behavior.

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

On Thu, Nov 20, 2025, at 6:18 AM, Akshay Joshi wrote:
>
>  Implemented in the suggested solution. Attached is the v5 patch for review.
>

I reviewed your patch and have some suggestions for this patch.

* You shouldn't include the property if the value is default. A long command
  adds nothing. Clarity? Tell someone that needs to select, copy and paste a
  long statement. It is a good goal to provide a short command to reconstruct
  the object. If you don't know why it didn't include CONNECTION LIMIT, it is
  time to check the manual again.

$ psql -AtqX -c "SELECT pg_get_database_ddl('postgres')" -d postgres
CREATE DATABASE postgres WITH OWNER = euler ENCODING = "UTF8" LC_COLLATE = "pt_BR.UTF-8" LC_CTYPE = "pt_BR.UTF-8"
COLLATION_VERSION= "2.36" LOCALE_PROVIDER = 'libc' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION LIMIT =
-1;

* Use single quotes. The encoding, locale, lc_collate, lc_type,
  collation_version and some other properties should use single quotes. The
  locale_provider doesn't need a single quote because it is an enum. See how
  pg_dumpall constructs the command. Use simple_quote_literal.

$ pg_dumpall --binary-upgrade | grep 'p5'
-- Database "p5" dump
-- Name: p5; Type: DATABASE; Schema: -; Owner: euler
CREATE DATABASE p5 WITH TEMPLATE = template0 OID = 16392 STRATEGY = FILE_COPY ENCODING = 'UTF8' LOCALE_PROVIDER = libc
LOCALE= 'en_US.UTF-8' COLLATION_VERSION = '2.36';
 
ALTER DATABASE p5 OWNER TO euler;
\connect p5

* OWNER. There is no guarantee that the owner exists in the cluster you will
  use this output. That's something that pg_dumpall treats separately (see
  above). Does it mean we should include the owner? No. We can make it an
  option.

* LOCALE. Why didn't you include it? I know there are some combinations that
  does not work together but this function can provide a minimal set of
  properties related to locale.

postgres=# CREATE DATABASE p6 LOCALE_PROVIDER builtin LOCALE 'C' TEMPLATE template0;
CREATE DATABASE

* STRATEGY. Although this is a runtime property, it should be an option.

* TEMPLATE. Ditto.

* options. Since I mentioned options for some properties (owner, strategy,
  template), these properties can be accommodated as a VARIADIC argument. The
  function signature can be something like

pg_get_database_ddl(oid, VARIADIC options text[])

I would include the pretty print into options too.

* Tabs. I don't think we use tabs to format output. Use spaces. A good practice
  is to use EXPLAIN style (2 spaces)and depending on the nesting, 4 spaces are
  fine too.

$ psql -AtqX -c "SELECT pg_get_database_ddl('postgres', true)" -d postgres
CREATE DATABASE postgres
    WITH OWNER = euler
        ENCODING = "UTF8"
        LC_COLLATE = "pt_BR.UTF-8"
        LC_CTYPE = "pt_BR.UTF-8"
        COLLATION_VERSION = "2.36"
        LOCALE_PROVIDER = 'libc'
        TABLESPACE = pg_default
        ALLOW_CONNECTIONS = true
        CONNECTION LIMIT = -1;

* permission. I don't think you need to check for permissions inside the
  function. I wouldn't want a different behavior than pg_dump(all). You can
  always adjust it in system_functions.sql.

* typo.

+--                                                                             
+-- Reconsturct DDL                                                             
+--

s/Reconsturct/Reconstruct/


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

On Thu, Dec 11, 2025 at 7:29 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Nov 20, 2025, at 6:18 AM, Akshay Joshi wrote:
> >
> >  Implemented in the suggested solution. Attached is the v5 patch for review.
> >
>
> I reviewed your patch and have some suggestions for this patch.
>
> * You shouldn't include the property if the value is default. A long command
>   adds nothing. Clarity? Tell someone that needs to select, copy and paste a
>   long statement. It is a good goal to provide a short command to reconstruct
>   the object. If you don't know why it didn't include CONNECTION LIMIT, it is
>   time to check the manual again.
>
> $ psql -AtqX -c "SELECT pg_get_database_ddl('postgres')" -d postgres
> CREATE DATABASE postgres WITH OWNER = euler ENCODING = "UTF8" LC_COLLATE = "pt_BR.UTF-8" LC_CTYPE = "pt_BR.UTF-8"
COLLATION_VERSION= "2.36" LOCALE_PROVIDER = 'libc' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION LIMIT =
-1;
>
Is there any way to obtain the default values directly from the source
code itself, or do I need to refer to the documentation? If we rely on
the documentation and compare against that, then in the future, if the
default values change, we would also need to update our logic
accordingly.

Constantly having to check the documentation for default values may
feel annoying to some users. Some users run queries with parameters
such as encoding, connection limit, and locale using their default
values. When they call the pg_get_database_ddl function, it
reconstructs the short command based on those defaults.

I am still open to updating my code.

> * Use single quotes. The encoding, locale, lc_collate, lc_type,
>   collation_version and some other properties should use single quotes. The
>   locale_provider doesn't need a single quote because it is an enum. See how
>   pg_dumpall constructs the command. Use simple_quote_literal.
>
I’ll update this in my next patch.

> $ pg_dumpall --binary-upgrade | grep 'p5'
> -- Database "p5" dump
> -- Name: p5; Type: DATABASE; Schema: -; Owner: euler
> CREATE DATABASE p5 WITH TEMPLATE = template0 OID = 16392 STRATEGY = FILE_COPY ENCODING = 'UTF8' LOCALE_PROVIDER =
libcLOCALE = 'en_US.UTF-8' COLLATION_VERSION = '2.36'; 
> ALTER DATABASE p5 OWNER TO euler;
> \connect p5
>
> * OWNER. There is no guarantee that the owner exists in the cluster you will
>   use this output. That's something that pg_dumpall treats separately (see
>   above). Does it mean we should include the owner? No. We can make it an
>   option.
>
If I understand correctly, the owner should be an option provided by
the caller of the function, and we reconstruct the Database DDL using
that specified owner. Is that right?
If so, then in my humble opinion, this is not truly a reconstruction
of the existing database object.

> * LOCALE. Why didn't you include it? I know there are some combinations that
>   does not work together but this function can provide a minimal set of
>   properties related to locale.
>
> postgres=# CREATE DATABASE p6 LOCALE_PROVIDER builtin LOCALE 'C' TEMPLATE template0;
> CREATE DATABASE
>
For LOCALE where the provider is libc, datlocale is NULL. For builtin
and icu, I have already added the following condition in the code:
if (!attrIsNull && dbForm->datlocprovider == COLLPROVIDER_BUILTIN)
    get_formatted_string(&buf, prettyFlags, 2, "BUILTIN_LOCALE = %s",
                         quote_identifier(TextDatumGetCString(dbValue)));
else if (!attrIsNull && dbForm->datlocprovider == COLLPROVIDER_ICU)
    get_formatted_string(&buf, prettyFlags, 2, "ICU_LOCALE = %s",
                         quote_identifier(TextDatumGetCString(dbValue)));

> * STRATEGY. Although this is a runtime property, it should be an option.
>
> * TEMPLATE. Ditto.
>
> * options. Since I mentioned options for some properties (owner, strategy,
>   template), these properties can be accommodated as a VARIADIC argument. The
>   function signature can be something like
>
> pg_get_database_ddl(oid, VARIADIC options text[])
>
> I would include the pretty print into options too.
>
Same comment as the one I gave for the Owner, if you are referring to
these as options to the function.

> * Tabs. I don't think we use tabs to format output. Use spaces. A good practice
>   is to use EXPLAIN style (2 spaces)and depending on the nesting, 4 spaces are
>   fine too.
>
> $ psql -AtqX -c "SELECT pg_get_database_ddl('postgres', true)" -d postgres
> CREATE DATABASE postgres
>         WITH OWNER = euler
>                 ENCODING = "UTF8"
>                 LC_COLLATE = "pt_BR.UTF-8"
>                 LC_CTYPE = "pt_BR.UTF-8"
>                 COLLATION_VERSION = "2.36"
>                 LOCALE_PROVIDER = 'libc'
>                 TABLESPACE = pg_default
>                 ALLOW_CONNECTIONS = true
>                 CONNECTION LIMIT = -1;
>
I received a review comment suggesting the use of tabs. I also looked
up PostgreSQL best practices on google, which recommend using tabs for
indentation and spaces for alignment. I’m open to updating my code
accordingly.

> * permission. I don't think you need to check for permissions inside the
>   function. I wouldn't want a different behavior than pg_dump(all). You can
>   always adjust it in system_functions.sql.
>
We’ve already had extensive discussions on this topic in the same
email thread, and ultimately we decided to add the permission check.

> * typo.
>
> +--
> +-- Reconsturct DDL
> +--
>
> s/Reconsturct/Reconstruct/
>
I’ll update this in my next patch.
>
> --
> Euler Taveira
> EDB   https://www.enterprisedb.com/

On Fri, Dec 12, 2025, at 7:52 AM, Akshay Joshi wrote:
> On Thu, Dec 11, 2025 at 7:29 PM Euler Taveira <euler@eulerto.com> wrote:
>>
> Is there any way to obtain the default values directly from the source
> code itself, or do I need to refer to the documentation? If we rely on
> the documentation and compare against that, then in the future, if the
> default values change, we would also need to update our logic
> accordingly.
>

No, you need to check the documentation. If you are changing the default value,
you are breaking compatibility; that rarely happens. If we are really concern
about this fact, you can add a test case that creates the object without
properties (all default values) and another with all default properties and
then compare the output.

> Constantly having to check the documentation for default values may
> feel annoying to some users. Some users run queries with parameters
> such as encoding, connection limit, and locale using their default
> values. When they call the pg_get_database_ddl function, it
> reconstructs the short command based on those defaults.
>

Encoding and locale, ok but I doubt about connection limit.

postgres=# SELECT current_user;
 current_user
--------------
 euler
(1 row)

postgres=# CREATE DATABASE foo;
CREATE DATABASE
postgres=# CREATE DATABASE bar OWNER euler;
CREATE DATABASE

When you are learning a new command, you generally don't set the default value
for a property just to be correct. I'm not saying this function shouldn't
include OWNER. I'm just suggesting it to be optional.  See some arguments
below.

>> * OWNER. There is no guarantee that the owner exists in the cluster you will
>>   use this output. That's something that pg_dumpall treats separately (see
>>   above). Does it mean we should include the owner? No. We can make it an
>>   option.
>>
> If I understand correctly, the owner should be an option provided by
> the caller of the function, and we reconstruct the Database DDL using
> that specified owner. Is that right?
> If so, then in my humble opinion, this is not truly a reconstruction
> of the existing database object.
>

No. My idea is to have something like the pg_dump --no-owner option. This is
important if you are transporting the objects from one cluster to another one.
Owner might be different. That's why I'm suggesting it should be optional. It
means flexibility. See pg_dump output format that always apply the OWNER as a
separate ALTER command.

>> * options. Since I mentioned options for some properties (owner, strategy,
>>   template), these properties can be accommodated as a VARIADIC argument. The
>>   function signature can be something like
>>
>> pg_get_database_ddl(oid, VARIADIC options text[])
>>
>> I would include the pretty print into options too.
>>
> Same comment as the one I gave for the Owner, if you are referring to
> these as options to the function.
>

Let me elaborate a bit. As I suggested you can control the output with options.
Why? Flexibility.

Why am I suggesting such a general purpose implementation? See some of the use
cases.

1. object DDL. Check DDL to recreate the object. It is not the exact DDL that
the user informed but it produces the same result.
2. clone tool. Clone the objects to recreate the environment for another
customer. These objects can be created in the same cluster or in another one.
(Of course, global objects don't apply for the same cluster.)
3. dump tool. Dump the commands to recreate the existing objects.
4. diff tool. There are tools like pgquarrel [1] that queries the catalog and
compare the results to create commands to turn the target database into the
source database. The general purpose functions can be used if the object
doesn't exist in the target database. (Of course, it doesn't apply for global
objects but again it is a good UI to have all of these pg_get_OBJECT_ddl
functions using the same approach.)
5. logical replication. These pg_get_OBJECT_ddl functions can be good
candidates to be used in the initial schema replication and even in the DDL
replication (if the object doesn't exist in the target database).

The "options" parameter is to get the DDL command to serve any of these use
cases. There are some properties in a certain object that you *don't* want for
whatever reason. See some --no-OBJECT options in pg_dump. Let's say you don't
want the TABLESPACE or the table access method while getting the CREATE TABLE
DDL because it is different in the other database.

> I received a review comment suggesting the use of tabs. I also looked
> up PostgreSQL best practices on google, which recommend using tabs for
> indentation and spaces for alignment. I’m open to updating my code
> accordingly.
>

I didn't check all of the possible output but the majority uses space instead
of tabs. Check psql. If you check the git history (git log --grep=tabs), you
will notice that tabs are removed from source code.

>> * permission. I don't think you need to check for permissions inside the
>>   function. I wouldn't want a different behavior than pg_dump(all). You can
>>   always adjust it in system_functions.sql.
>>
> We’ve already had extensive discussions on this topic in the same
> email thread, and ultimately we decided to add the permission check.
>

That's fair. Again, I expect that all of these pg_get_OBJECT_ddl functions use
the same approach. We can always relax this restriction in the future.


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