public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Akshay Joshi <[email protected]>
To: Álvaro Herrera <[email protected]>
Cc: Japin Li <[email protected]>
Cc: Quan Zongliang <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement
Date: Tue, 18 Nov 2025 08:28:23 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CANxoLDdTfsmmx6vMcZfdvW+5GSUjE9Fs5O4Jwiy3aGHiV+Vz8g@mail.gmail.com>
References: <CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com>
<[email protected]>
<[email protected]>
<CANxoLDfWexTBBVgSOabeh-z=-0BTQCEst_E037tej+DLJxtFqw@mail.gmail.com>
<SY0P300MB153948953051E054E8F69487B6CAA@SY0P300MB1539.AUSP300.PROD.OUTLOOK.COM>
<CANxoLDdTfsmmx6vMcZfdvW+5GSUjE9Fs5O4Jwiy3aGHiV+Vz8g@mail.gmail.com>
Hi Akshay,
I just reviewed v3 and got some comments:
> On Nov 17, 2025, at 22:34, Akshay Joshi <[email protected]> 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.
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 just did 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 the second 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_CNONNECTIONS should be omitted. For CONNECTION LIMIT, you already have a logic to omit it but the logic has some problem as 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 should be common for all those patches. Maybe Alvaro can help here, pushing a common function that is used to format DDL and requesting all patches to use the function.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
view thread (38+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox