MIME-Version: 1.0
References: <CANxoLDdPqyTLyYhBnekFYkaSn_8DBdtf32cH=MLNJc=+BQP=MQ@mail.gmail.com>
 <202511191045.tckydhpoxumx@alvherre.pgsql> <CANxoLDeEzSvC8hy7oF=qZZGVD---MDwxesdoQnAQMuJQGOSaJw@mail.gmail.com>
 <4e60bcae-8222-4e1f-8e5b-d73b59c93304@app.fastmail.com> <CANxoLDeAv5M-HcVnwN6q3RNaBeXRfkR7_3LAvZ=pfLqc8=N8vw@mail.gmail.com>
 <4c695e76-5ab7-449f-8060-76518dd41468@app.fastmail.com> <c2437767-c131-43a4-b779-37c391cf4fe3@dunslane.net>
 <CANxoLDcF=eqXB59WcTqCwrp6u5Qw0bUjVSTbRApquSvsYLcPWQ@mail.gmail.com>
 <CANxoLDd2iMLGc1WsNFywB_jZmCYJON-6g1awwsC_S1Q-PRJDVw@mail.gmail.com>
 <CANxoLDfvx0t-wMNbtbtnRwcV9wNW58PY3vOSKSGukhGgw5-e1A@mail.gmail.com>
 <CANxoLDd73eDHykBquqqButBp73mHudyM-3iQnYp6FmPnetZb-g@mail.gmail.com>
 <CAAJ_b9421F_Ox5SEvMp537UQvJyvZhWuhQ9=i=6n73KZBDDa+Q@mail.gmail.com> <CANxoLDdkzxGRnMpHrizVRn-Y7HDc6oPV4P4uWmMvf=TU2a8RTw@mail.gmail.com>
In-Reply-To: <CANxoLDdkzxGRnMpHrizVRn-Y7HDc6oPV4P4uWmMvf=TU2a8RTw@mail.gmail.com>
From: Amul Sul <sulamul@gmail.com>
Date: Tue, 24 Feb 2026 09:09:18 +0530
Message-ID: <CAAJ_b96TaEa7eRkNOy323FJR2HFRA_rQCO=c5eviimE6RbhW4w@mail.gmail.com>
Subject: Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE
 DATABASE statement
To: Akshay Joshi <akshay.joshi@enterprisedb.com>
Cc: Andrew Dunstan <andrew@dunslane.net>, Euler Taveira <euler@eulerto.com>, 
	=?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>, 
	Chao Li <li.evan.chao@gmail.com>, japin <japinli@hotmail.com>, 
	Quan Zongliang <quanzongliang@yeah.net>, pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAAJ_b96TaEa7eRkNOy323FJR2HFRA_rQCO%3Dc5eviimE6RbhW4w%40mail.gmail.com>
Precedence: bulk

On Mon, Feb 23, 2026 at 7:27=E2=80=AFPM Akshay Joshi
<akshay.joshi@enterprisedb.com> wrote:
>
> On Mon, Feb 23, 2026 at 6:50=E2=80=AFPM Amul Sul <sulamul@gmail.com> wrot=
e:
> > [....]
> > /* Standard conversion of a "bool pretty" option to detailed flags */
> >  #define GET_PRETTY_FLAGS(pretty) \
> >     ((pretty) ? (PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | PRETTYFLAG_SCHE=
MA) \
> >      : PRETTYFLAG_INDENT)
> >
> > +#define GET_DDL_PRETTY_FLAGS(pretty) \
> > +   ((pretty) ? (PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | PRETTYFLAG_SCHE=
MA) \
> > +    : 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) !=3D 0))
> >
> >
> > Comment doesn't quite match the logic in the condition.
>
> Will update this in my next patch. When we decide which approach to follo=
w:
> 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=E2=80=99s 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