public inbox for [email protected]
help / color / mirror / Atom feedFrom: Japin Li <[email protected]>
To: Akshay Joshi <[email protected]>
Cc: Euler Taveira <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Amul Sul <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Quan Zongliang <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement
Date: Wed, 04 Mar 2026 16:39:06 +0800
Message-ID: <SY7PR01MB10921EA37563D7A65CFF0D771B67CA@SY7PR01MB10921.ausprd01.prod.outlook.com> (raw)
In-Reply-To: <CANxoLDfR-8nOYetoSKiJ9yVvMkXodVRqR+e=7CSEwiJTrU7yrA@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CANxoLDfR-8nOYetoSKiJ9yVvMkXodVRqR+e=7CSEwiJTrU7yrA@mail.gmail.com>
On Fri, 27 Feb 2026 at 17:52, Akshay Joshi <[email protected]> wrote:
> Following suggestions from Alvaro and Mark, I have re-implemented this feature using variadic
> function argument pairs.
> This patch incorporates Mark Wong’s documentation updates, Amul’s one review comment, and the
> majority of Euler’s comments.
>
> New changes:
> - Supports flexible DDL options as key-value pairs:
> - pretty - Format output for readability
> - owner - Include OWNER clause
> - tablespace - Include TABLESPACE clause
> - defaults - Include parameters even if it is set to default value.
> - Option values accept: 'yes'/'on'/true/'1' (or their negatives)
>
> Usage Examples:
> -- Basic usage with options
> SELECT pg_get_database_ddl('mydb', 'owner', 'yes', 'defaults', 'yes');
> -- Pretty-printed output without owner and tablespace
> SELECT pg_get_database_ddl('mydb', 'owner', 'no', 'tablespace', 'no', 'pretty', 'on');
> -- Using boolean values
> SELECT pg_get_database_ddl('mydb', 'owner', false, 'defaults', true);
> -- Using OID
> SELECT pg_get_database_ddl(16384, 'pretty', 'yes');
>
> Note: To keep things clean, I’ve moved the logic into two generic functions (get_formatted_string
> and parse_ddl_options) and a common DDLOptionDef struct. This should simplify the work for the
> rest of the pg_get_<object>_ddl patches.
>
> Attached is the v9 patch which is ready for review.
>
> On Thu, Feb 26, 2026 at 2:49 AM Euler Taveira <[email protected]> wrote:
>
> On Wed, Feb 25, 2026, at 8:53 AM, Álvaro Herrera wrote:
> >
> > I'm surprised to not have seen an update on this topic following the
> > discovery by Mark Wong that commit d32d1463995c (in branch 18) already
> > established a convention for passing arguments to functions: use argument
> > pairs to variadic functions, the way pg_restore_relation_stats() and
> > pg_restore_attribute_stats() work. While I like my previous suggestion
> > of using DefElems better, I think it's more sensible to follow this
> > established precedent and not innovate on this.
> >
>
> This convention is much older than the referred commit. It predates from the
> logical decoding (commit b89e151054a0). See pg_logical_slot_get_changes_guts()
> that is an internal function for pg_logical_slot_FOO_changes(). It seems a
> good idea to have a central function to validate the variadic parameter for all
> of these functions.
>
Thanks for updating the patch, here are some comments on v9.
1.
+ uint64 flag; /* Flag to set */
Do we actually need 64 bits for this flag field?
2.
+ /* Indent with spaces */
+ for (int i = 0; i < nSpaces; i++)
+ {
+ appendStringInfoChar(buf, ' ');
+ }
How about using appendStringInfoSpaces(buf, nSpaces) instead?
3.
+ /* If no options provided (VARIADIC NULL), return the empty bitmask */
+ if (nargs < 0)
+ return flags;
+
+ ...
+
+ /* No arguments provided */
+ if (nargs == 0)
+ return flags;
These two conditions are identical — how about just `if (nargs <= 0)`?
4.
+ /* Arguments must come in name/value pairs */
+ if (nargs % 2 != 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument list must have even number of elements"),
+ errhint("The arguments of %s must consist of alternating keys and values.",
+ "pg_get_database_ddl()")));
Should we align this with stats_fill_fcinfo_from_arg_pairs()?
Suggested wording:
errmsg("variadic arguments must be name/value pairs")
5.
+ /* Key must not be null */
+ if (nulls[i])
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument %d: key must not be null", i + 1)));
+
Suggested wording:
errmsg("name at variadic position %d is null")
6.
+ /* Key must be text type */
+ if (types[i] != TEXTOID)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument %d: key must be text type", i + 1)));
Suggested wording:
errmsg("name at variadic position %d has type %s, expected type %s")
7.
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument %d: value for key \"%s\" must be boolean or text type",
+ i + 2, name)));
Suggested wording:
errmsg("argument \"%s\" has type %s, execpted type boolean or text")
See stats_check_arg_type().
8.
+ for (i = 0; i < nargs; i += 2)
+ {
We can narrow the scope of `i` by declaring it in the for initializer.
9.
+ {
+ bool found = false;
+ int j;
+
+ for (j = 0; j < lengthof(ddl_option_defs); j++)
+ {
Minor style improvements:
- We can (and should) declare `j` inside its for-loop initializer, just like `i`.
- Move the declaration of `found` up to the top of the outer for-loop scope.
This allows us to remove the unnecessary braces around the loop body.
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
view thread (41+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement
In-Reply-To: <SY7PR01MB10921EA37563D7A65CFF0D771B67CA@SY7PR01MB10921.ausprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox