Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement - Mailing list pgsql-hackers

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/