Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE
 DATABASE statement
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CANxoLDdTfsmmx6vMcZfdvW+5GSUjE9Fs5O4Jwiy3aGHiV+Vz8g@mail.gmail.com>
Date: Tue, 18 Nov 2025 08:28:23 +0800
Cc: Japin Li <japinli@hotmail.com>,
 Quan Zongliang <quanzongliang@yeah.net>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <FDC0082F-3AB6-4E0F-942A-44831860F7D5@gmail.com>
References: 
 <CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com>
 <d6a8bb90-9b55-4124-8c80-b5a5c1ee460c@yeah.net>
 <7daf5cec-4eae-48e0-883e-684476b57531@yeah.net>
 <CANxoLDfWexTBBVgSOabeh-z=-0BTQCEst_E037tej+DLJxtFqw@mail.gmail.com>
 <SY0P300MB153948953051E054E8F69487B6CAA@SY0P300MB1539.AUSP300.PROD.OUTLOOK.COM>
 <CANxoLDdTfsmmx6vMcZfdvW+5GSUjE9Fs5O4Jwiy3aGHiV+Vz8g@mail.gmail.com>
To: Akshay Joshi <akshay.joshi@enterprisedb.com>,
 =?utf-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>
Archived-At: 
 <https://www.postgresql.org/message-id/FDC0082F-3AB6-4E0F-942A-44831860F7D5%40gmail.com>
Precedence: bulk

Hi Akshay,

I just reviewed v3 and got some comments:

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


1 - ruleutils.c
```
+	if (dbForm->datconnlimit !=3D 0)
+		get_formatted_string(&buf, prettyFlags, 1, "CONNECTION =
LIMIT =3D %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 =3D =
%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 =3D object_aclcheck(DatabaseRelationId, dbOid, =
GetUserId(),
+								=
ACL_CONNECT);
+	if (aclresult !=3D ACLCHECK_OK)
+	{
+		aclcheck_error(aclresult, OBJECT_DATABASE,
+					   get_database_name(dbOid));
+	}
```

I don=E2=80=99t 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=20

# Without any privilege grant, the user can get ddl of the system =
database, which seems not good
evantest=3D> select pg_get_database_ddl('postgres', true);
        pg_get_database_ddl
------------------------------------
 CREATE DATABASE postgres          +
         WITH                      +
         OWNER =3D chaol             +
         ENCODING =3D "UTF8"         +
         LC_COLLATE =3D "en_US.UTF-8"+
         LC_CTYPE =3D "en_US.UTF-8"  +
         LOCALE_PROVIDER =3D 'libc'  +
         TABLESPACE =3D pg_default   +
         ALLOW_CONNECTIONS =3D true  +
         CONNECTION LIMIT =3D -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, =E2=80=9C+=E2=80=9D in =
the end of every line is weird.

6 - =E2=80=9CWITH=E2=80=9D has the same indent as the parameters, which =
doesn=E2=80=99t good look. If we look at the doc =
https://www.postgresql.org/docs/18/sql-createdatabase.html, =E2=80=9CWITH=E2=
=80=9D 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=3D> select pg_get_database_ddl('evantest', true);
        pg_get_database_ddl
------------------------------------
 CREATE DATABASE evantest          +
         WITH                      +
         OWNER =3D chaol             +
         ENCODING =3D "UTF8"         +
         LC_COLLATE =3D "en_US.UTF-8"+
         LC_CTYPE =3D "en_US.UTF-8"  +
         LOCALE_PROVIDER =3D 'libc'  +
         TABLESPACE =3D pg_default   +
         ALLOW_CONNECTIONS =3D true  +
         CONNECTION LIMIT =3D -1;
(1 row)
```

I created the database =E2=80=9Cevantest=E2=80=9D 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=E2=80=99t 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/