public inbox for [email protected]
help / color / mirror / Atom feedFrom: Japin Li <[email protected]>
To: Akshay Joshi <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Álvaro Herrera <[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: Wed, 19 Nov 2025 18:17:49 +0800
Message-ID: <SY0P300MB15399C568056B4F187A846FDB6D7A@SY0P300MB1539.AUSP300.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <CANxoLDdPqyTLyYhBnekFYkaSn_8DBdtf32cH=MLNJc=+BQP=MQ@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>
<[email protected]>
<CANxoLDdPqyTLyYhBnekFYkaSn_8DBdtf32cH=MLNJc=+BQP=MQ@mail.gmail.com>
Hi Akshay,
Thanks for updating the patch.
On Tue, 18 Nov 2025 at 13:33, Akshay Joshi <[email protected]> wrote:
> Hi Chao
>
> Thanks for reviewing my patch.
>
> On Tue, Nov 18, 2025 at 5:59 AM Chao Li <[email protected]> wrote:
>
> 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.
>
> 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.
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: <SY0P300MB15399C568056B4F187A846FDB6D7A@SY0P300MB1539.AUSP300.PROD.OUTLOOK.COM>
* 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