Thread

Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement

Akshay Joshi <akshay.joshi@enterprisedb.com> — 2025-11-19T11:06:34Z

On Wed, Nov 19, 2025 at 3:48 PM Japin Li <japinli@hotmail.com> wrote:

>
> 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)
>

The FormData_pg_database structure does not expose the database *STRATEGY*,
and reconstructing the *OID* serves no practical purpose. If the majority
agrees, I can extend the DDL to include OID.

>
> 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));
> +       }
>

   Will fix this in the next patch.

>
> > >
> > > 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.
>