On Wed, Jan 21, 2026 at 3:02 PM Akshay Joshi
<[email protected]> 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 suitable and 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
