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

On Thu, Dec 11, 2025 at 7:29 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Thu, Nov 20, 2025, at 6:18 AM, Akshay Joshi wrote:
> >
> > Implemented in the suggested solution. Attached is the v5 patch for review.
> >
>
> I reviewed your patch and have some suggestions for this patch.
>
> * You shouldn't include the property if the value is default. A long command
> adds nothing. Clarity? Tell someone that needs to select, copy and paste a
> long statement. It is a good goal to provide a short command to reconstruct
> the object. If you don't know why it didn't include CONNECTION LIMIT, it is
> time to check the manual again.
>
> $ psql -AtqX -c "SELECT pg_get_database_ddl('postgres')" -d postgres
> CREATE DATABASE postgres WITH OWNER = euler ENCODING = "UTF8" LC_COLLATE = "pt_BR.UTF-8" LC_CTYPE = "pt_BR.UTF-8" COLLATION_VERSION = "2.36" LOCALE_PROVIDER = 'libc' TABLESPACE = pg_default ALLOW_CONNECTIONS = true CONNECTION LIMIT = -1;
>
Is there any way to obtain the default values directly from the source
code itself, or do I need to refer to the documentation? If we rely on
the documentation and compare against that, then in the future, if the
default values change, we would also need to update our logic
accordingly.

Constantly having to check the documentation for default values may
feel annoying to some users. Some users run queries with parameters
such as encoding, connection limit, and locale using their default
values. When they call the pg_get_database_ddl function, it
reconstructs the short command based on those defaults.

I am still open to updating my code.

> * Use single quotes. The encoding, locale, lc_collate, lc_type,
> collation_version and some other properties should use single quotes. The
> locale_provider doesn't need a single quote because it is an enum. See how
> pg_dumpall constructs the command. Use simple_quote_literal.
>
I’ll update this in my next patch.

> $ pg_dumpall --binary-upgrade | grep 'p5'
> -- Database "p5" dump
> -- Name: p5; Type: DATABASE; Schema: -; Owner: euler
> CREATE DATABASE p5 WITH TEMPLATE = template0 OID = 16392 STRATEGY = FILE_COPY ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8' COLLATION_VERSION = '2.36';
> ALTER DATABASE p5 OWNER TO euler;
> \connect p5
>
> * OWNER. There is no guarantee that the owner exists in the cluster you will
> use this output. That's something that pg_dumpall treats separately (see
> above). Does it mean we should include the owner? No. We can make it an
> option.
>
If I understand correctly, the owner should be an option provided by
the caller of the function, and we reconstruct the Database DDL using
that specified owner. Is that right?
If so, then in my humble opinion, this is not truly a reconstruction
of the existing database object.

> * LOCALE. Why didn't you include it? I know there are some combinations that
> does not work together but this function can provide a minimal set of
> properties related to locale.
>
> postgres=# CREATE DATABASE p6 LOCALE_PROVIDER builtin LOCALE 'C' TEMPLATE template0;
> CREATE DATABASE
>
For LOCALE where the provider is libc, datlocale is NULL. For builtin
and icu, I have already added the following condition in the code:
if (!attrIsNull && dbForm->datlocprovider == COLLPROVIDER_BUILTIN)
get_formatted_string(&buf, prettyFlags, 2, "BUILTIN_LOCALE = %s",
quote_identifier(TextDatumGetCString(dbValue)));
else if (!attrIsNull && dbForm->datlocprovider == COLLPROVIDER_ICU)
get_formatted_string(&buf, prettyFlags, 2, "ICU_LOCALE = %s",
quote_identifier(TextDatumGetCString(dbValue)));

> * STRATEGY. Although this is a runtime property, it should be an option.
>
> * TEMPLATE. Ditto.
>
> * options. Since I mentioned options for some properties (owner, strategy,
> template), these properties can be accommodated as a VARIADIC argument. The
> function signature can be something like
>
> pg_get_database_ddl(oid, VARIADIC options text[])
>
> I would include the pretty print into options too.
>
Same comment as the one I gave for the Owner, if you are referring to
these as options to the function.

> * Tabs. I don't think we use tabs to format output. Use spaces. A good practice
> is to use EXPLAIN style (2 spaces)and depending on the nesting, 4 spaces are
> fine too.
>
> $ psql -AtqX -c "SELECT pg_get_database_ddl('postgres', true)" -d postgres
> CREATE DATABASE postgres
> WITH OWNER = euler
> ENCODING = "UTF8"
> LC_COLLATE = "pt_BR.UTF-8"
> LC_CTYPE = "pt_BR.UTF-8"
> COLLATION_VERSION = "2.36"
> LOCALE_PROVIDER = 'libc'
> TABLESPACE = pg_default
> ALLOW_CONNECTIONS = true
> CONNECTION LIMIT = -1;
>
I received a review comment suggesting the use of tabs. I also looked
up PostgreSQL best practices on google, which recommend using tabs for
indentation and spaces for alignment. I’m open to updating my code
accordingly.

> * permission. I don't think you need to check for permissions inside the
> function. I wouldn't want a different behavior than pg_dump(all). You can
> always adjust it in system_functions.sql.
>
We’ve already had extensive discussions on this topic in the same
email thread, and ultimately we decided to add the permission check.

> * typo.
>
> +--
> +-- Reconsturct DDL
> +--
>
> s/Reconsturct/Reconstruct/
>
I’ll update this in my next patch.
>
> --
> Euler Taveira
> EDB https://www.enterprisedb.com/