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

On Wed, Mar 11, 2026 at 3:35 AM Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>
wrote:

> Hello
>
> Is ruleutils.c the best place for this function?
>
> It's already huge, and it has a different scope: "Functions to convert
> stored expressions/querytrees back to source text"
>

Created the ddlutils.c file.

>
> + /* Fetch the value of COLLATION_VERSION */
> + dbvalue = SysCacheGetAttr(DATABASEOID, tuple_database,
> + Anum_pg_database_datcollversion, &attr_isnull);
> + if (!attr_isnull)
> + get_formatted_string(&buf, pretty_flags, 8, "COLLATION_VERSION = %s",
> + quote_literal_cstr(TextDatumGetCString(dbvalue)));
>
> pg_dumpall only shows this for binary upgrade, otherwise skips it. Is
> it okay for this command to print it by default, shouldn't it depend
> on is_with_defaults or something similar?
>

Shows only when `is_with_defaults` is true.

>
> +#ifndef DDL_DEFAULTS_H
> +#define DDL_DEFAULTS_H
> +
> +static const struct
> +{
> + struct
> + {
> ....
>
> This file seems strange. A static const struct in a header with
> uppercase names doesn't seem to follow postgres conventions?
> DATCONNLIMIT_UNLIMITED alredy exists as a definition, and probably
> should be used instead or referenced, or the existing uses should
> refer to the new way of defining it.
>

Removed the header file and implemented an alternative logic. Note that
a similar file may be necessary in the future to handle default values for
other pg_get_<object>_ddl.

>
> + /* Fetch the value of LC_COLLATE */
> + dbvalue = SysCacheGetAttr(DATABASEOID, tuple_database,
> + Anum_pg_database_datcollate, &attr_isnull);
> + if (!attr_isnull)
> + get_formatted_string(&buf, pretty_flags, 8, "LC_COLLATE = %s",
> + quote_literal_cstr(TextDatumGetCString(dbvalue)));
> + /* Fetch the value of LC_CTYPE */
> + dbvalue = SysCacheGetAttr(DATABASEOID, tuple_database,
> + Anum_pg_database_datctype, &attr_isnull);
>
> Can these be ever nulls?
>
> Also, pg_dump only emits LOCALE if ctype==collate, shouldn't this
> follow the same pattern?
>
> + else if (is_with_defaults)
> + get_formatted_string(&buf, pretty_flags, 8, "LOCALE_PROVIDER = libc");
>
> Doesn't pg_dump always emit this? Shouldn't this function follow the
> same convention? Emitting it seems to be a safer default, in case
> postgres ever changes this.
>
> + /* Build the CREATE DATABASE statement */
> + appendStringInfo(&buf, "CREATE DATABASE %s",
> + quote_identifier(dbform->datname.data));
> + get_formatted_string(&buf, pretty_flags, 4, "WITH");
>
> Shouldn't we only emit "WITH" if it is actually followed by something,
> not unconditionally?
>
> +/*
> + * get_formatted_string
> + *
> + * Return a formatted version of the string.
>
> But it's a void function.
>
>
> + else if (!attr_isnull)
> + get_formatted_string(&buf, pretty_flags, 8, "LOCALE = %s",
> + quote_literal_cstr(TextDatumGetCString(dbvalue)));
> +
>
> Can this ever happen, shouldn't it be an assertion instead?
>

Fixed all the preceding review comments.
Attached is the *v15 patch*, now ready for further review.