On Mon, Feb 23, 2026 at 7:27 PM Akshay Joshi
<[email protected]> wrote:
>
> On Mon, Feb 23, 2026 at 6:50 PM Amul Sul <[email protected]> wrote:
> > [....]
> > /* 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.
>
However, in the patch, you're calling it conditionally; specifically,
when the pretty argument is true, it gets called with a value of 1.
Also, instead of 1 it should be 'true'.
> >
> > +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 / 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.
>
I think you are correct -- not needed.
> >
> > +/**
> > + * 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?
>
Yes, that should be a completely separate patch. You can submit it in
this thread as part of the same series.
> >
> > + /* 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
>
That’s a bit subjective, as different people will likely have
different opinions. I prefer the first version without the -- prefix,
since the counterpart would be the default behavior. For example, if
"no-tablespace" is not specified, the tablespace would be included in
the DDL dump by default.
Regards,
Amul
