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

On Fri, 27 Feb 2026 at 17:52, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
> Following suggestions from Alvaro and Mark, I have re-implemented this feature using variadic
> function argument pairs.
> This patch incorporates Mark Wong’s documentation updates, Amul’s one review comment, and the
> majority of Euler’s comments.
>
> New changes:
> - Supports flexible DDL options as key-value pairs:
>     - pretty - Format output for readability
>     - owner - Include OWNER clause
>     - tablespace - Include TABLESPACE clause
>     - defaults - Include parameters even if it is set to default value.
> - Option values accept: 'yes'/'on'/true/'1' (or their negatives)
>
> Usage Examples:
>   -- Basic usage with options
>   SELECT pg_get_database_ddl('mydb', 'owner', 'yes', 'defaults', 'yes');
>   -- Pretty-printed output without owner and tablespace
>   SELECT pg_get_database_ddl('mydb', 'owner', 'no', 'tablespace', 'no', 'pretty', 'on');
>   -- Using boolean values
>   SELECT pg_get_database_ddl('mydb', 'owner', false, 'defaults', true);
>   -- Using OID
>   SELECT pg_get_database_ddl(16384, 'pretty', 'yes');
>
> Note: To keep things clean, I’ve moved the logic into two generic functions (get_formatted_string
> and parse_ddl_options) and a common DDLOptionDef struct. This should simplify the work for the
> rest of the pg_get_<object>_ddl patches.
>
> Attached is the v9 patch which is ready for review.
>
> On Thu, Feb 26, 2026 at 2:49 AM Euler Taveira <euler@eulerto.com> wrote:
>
>  On Wed, Feb 25, 2026, at 8:53 AM, Álvaro Herrera wrote:
>  >
>  > I'm surprised to not have seen an update on this topic following the
>  > discovery by Mark Wong that commit d32d1463995c (in branch 18) already
>  > established a convention for passing arguments to functions: use argument
>  > pairs to variadic functions, the way pg_restore_relation_stats() and
>  > pg_restore_attribute_stats() work.  While I like my previous suggestion
>  > of using DefElems better, I think it's more sensible to follow this
>  > established precedent and not innovate on this.
>  >
>
>  This convention is much older than the referred commit. It predates from the
>  logical decoding (commit b89e151054a0). See pg_logical_slot_get_changes_guts()
>  that is an internal function for pg_logical_slot_FOO_changes().  It seems a
>  good idea to have a central function to validate the variadic parameter for all
>  of these functions.
>

Thanks for updating the patch, here are some comments on v9.

1.
+    uint64        flag;            /* Flag to set */

Do we actually need 64 bits for this flag field?

2.
+        /* Indent with spaces */
+        for (int i = 0; i < nSpaces; i++)
+        {
+            appendStringInfoChar(buf, ' ');
+        }

How about using appendStringInfoSpaces(buf, nSpaces) instead?

3.
+    /* If no options provided (VARIADIC NULL), return the empty bitmask */
+    if (nargs < 0)
+        return flags;
+
+    ...
+
+    /* No arguments provided */
+    if (nargs == 0)
+        return flags;

These two conditions are identical — how about just `if (nargs <= 0)`?

4.
+    /* Arguments must come in name/value pairs */
+    if (nargs % 2 != 0)
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("argument list must have even number of elements"),
+                 errhint("The arguments of %s must consist of alternating keys and values.",
+                         "pg_get_database_ddl()")));

Should we align this with stats_fill_fcinfo_from_arg_pairs()?

Suggested wording:
    errmsg("variadic arguments must be name/value pairs")

5.
+        /* Key must not be null */
+        if (nulls[i])
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                     errmsg("argument %d: key must not be null", i + 1)));
+

Suggested wording:

    errmsg("name at variadic position %d is null")

6.
+        /* Key must be text type */
+        if (types[i] != TEXTOID)
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                     errmsg("argument %d: key must be text type", i + 1)));

Suggested wording:

    errmsg("name at variadic position %d has type %s, expected type %s")

7.
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                     errmsg("argument %d: value for key \"%s\" must be boolean or text type",
+                            i + 2, name)));

Suggested wording:

    errmsg("argument \"%s\" has type %s, execpted type boolean or text")

See stats_check_arg_type().

8.
+    for (i = 0; i < nargs; i += 2)
+    {

We can narrow the scope of `i` by declaring it in the for initializer.

9.
+        {
+            bool        found = false;
+            int         j;
+
+            for (j = 0; j < lengthof(ddl_option_defs); j++)
+            {

Minor style improvements:

- We can (and should) declare `j` inside its for-loop initializer, just like `i`.
- Move the declaration of `found` up to the top of the outer for-loop scope.
  This allows us to remove the unnecessary braces around the loop body.

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.