On Wed, 04 Mar 2026 at 16:39, Japin Li <[email protected]> wrote:
> 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.
>
After playing with this patch, I’m seeing the following output:
# select pg_get_database_ddl('postgres'::regdatabase, 'defaults', true,
'pretty', true, 'pretty', false);
pg_get_database_ddl
------------------------------------
CREATE DATABASE postgres +
WITH +
OWNER = japin +
ENCODING = 'UTF8' +
LC_COLLATE = 'en_US.UTF-8'+
LC_CTYPE = 'en_US.UTF-8' +
COLLATION_VERSION = '2.39'+
LOCALE_PROVIDER = libc +
TABLESPACE = pg_default +
ALLOW_CONNECTIONS = true +
CONNECTION LIMIT = -1;
(1 row)
Is this the expected behavior?
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
