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

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.