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

On Wed, Jan 21, 2026 at 3:02 PM Akshay Joshi
<akshay.joshi@enterprisedb.com> wrote:
>
> In my previous email, I included two different patches (for two separate approaches) from different branches. As a
result,CommitFest is indicating that a rebase is required. 
>
> Apologies for the inconvenience, I’m still getting familiar with the process.
>
> Attached are the patches, layered one on top of the other, representing two approaches:
>
> Double Dash: v8-0001-Add-pg_get_database_ddl-function-to-reconstruct-double-dash.patch
> DefElem (Key-Value): v8-0002-Add-pg_get_database_ddl-function-to-reconstruct-DefElem.patch
>
> I am now submitting the v8 patches, which are ready for review. Please let me know which approach you find more
suitableand preferable. 
>
>

I took a quick look at the v8 patches. I am not yet sure which
approach is superior, but here are some review for 0001:

+      <para>
+      <literal>pretty</literal>: Adds newlines and indentation for
better readability.
+      </para>

Could we use phrasing similar to what is used elsewhere in the
documentation? For reference, please see sgml/func/func-info.sgml
--

+    <listitem>
+      <para>
+      <literal>--no-owner</literal>: Omits the
<literal>OWNER</literal> clause from
+      the reconstructed statement.
+      </para>
+    </listitem>
+    <listitem>
+      <para>
+      <literal>--no-tablespace</literal>: Omits the
<literal>TABLESPACE</literal> clause.
+      </para>
+    </listitem>
+    <listitem>
+      <para>
+      <literal>--with-defaults</literal>: Includes clauses for
parameters that are
+      currently at their default values (e.g., <literal>CONNECTION
LIMIT -1</literal>),
+      which are normally omitted for brevity.
+      </para>
+    </listitem>

Formatting is inconsistent; some options include the -- prefix while
others do not. I think we should simply avoid the prefix entirely.
--

/* Standard conversion of a "bool pretty" option to detailed flags */
 #define GET_PRETTY_FLAGS(pretty) \
    ((pretty) ? (PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | PRETTYFLAG_SCHEMA) \
     : PRETTYFLAG_INDENT)

+#define GET_DDL_PRETTY_FLAGS(pretty) \
+   ((pretty) ? (PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | PRETTYFLAG_SCHEMA) \
+    : 0)
+

I am a bit confused -- why do we need a separate
GET_DDL_PRETTY_FLAGS() function?
--

+CREATE OR REPLACE FUNCTION
+  pg_get_database_ddl(database_id regdatabase, VARIADIC ddl_options
text[] DEFAULT '{}')
+RETURNS text
+LANGUAGE internal
+AS 'pg_get_database_ddl';
+

I don't see any REVOKE EXECUTE ON FUNCTION for this function.
--

+/**
+ * parse_ddl_options - Generic helper to parse variadic text options
+ * ddl_options: The ArrayType from PG_GETARG_ARRAYTYPE_P
+ * flags: Bitmask to set options while parsing DDL options.
+ */
+static uint64
+parse_ddl_options(ArrayType *ddl_options)

I believe this should be in a separate patch so that it can be reused
by others working on similar projects. It shouldn't be specific to
this patch.
--

+       /* If it's with defaults, we skip default encoding check */
+       if (is_with_defaults ||
+           (pg_strcasecmp(pg_encoding_to_char(dbform->encoding),
+                          DDL_DEFAULTS.DATABASE.ENCODING) != 0))


Comment doesn't quite match the logic in the condition.

Regards,
Amul