On Mon, Feb 23, 2026 at 6:50 PM Amul Sul <[email protected]> wrote:
>
> 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.
Mark Wong has provided a patch (v8-0003-doc-suggestions.patch) on top
of my changes, which should resolve the issues mentioned above.
> --
>
> /* 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?
> --
If we use the existing GET_PRETTY_FLAGS, it returns PRETTYFLAG_INDENT
even when pretty is false. However, for Reconstruction DDL, do not
apply indentation if pretty is false.
>
> +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.
> --
Other pg_get_*def functions don't have REVOKE: Functions like
pg_get_viewdef, pg_get_indexdef, pg_get_functiondef,
pg_get_triggerdef, and pg_get_constraintdef are all publicly
accessible none have it. pg_get_database_ddl is purely informational;
it reconstructs CREATE DATABASE DDL from data already visible in the
pg_database system catalog. Any user who can query pg_database already
has access to this information.
I can add, if the above explanation is wrong. Just let me know.
>
> +/**
> + * 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.
> --
I have added two generic functions, parse_ddl_options and
get_formatted_string, which will be required by future
pg_get_<object>_ddl patches. Is it acceptable to submit a patch
containing only these generic functions before they are actively used?
>
> + /* 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.
Will update this in my next patch. When we decide which approach to follow:
1) Double Dash:
v8-0001-Add-pg_get_database_ddl-function-to-reconstruct-double-dash.patch
2) DefElem (Key-Value):
v8-0002-Add-pg_get_database_ddl-function-to-reconstruct-DefElem.patch
>
> Regards,
> Amul
