I like the idea for this patch and find this useful.
I have few comments for the patch, looking at the test cases added in the
patch, it is not clear to me what is the default value for each of the
options. For example, in
+-- With No Tablespace
+SELECT ddl_filter(pg_get_database_ddl('regression_utf8', 'defaults', true,
'tablespace', false));
+
ddl_filter
+----------------------------------------------------------------------------------------------------------------------------------------------------------
+ CREATE DATABASE regression_utf8 WITH OWNER = regress_datdba_after
ENCODING = 'UTF8' ALLOW_CONNECTIONS = true CONNECTION LIMIT = 123
IS_TEMPLATE = false;
+(1 row)
+
owner option is omitted but the output contains the owner information. So
does it mean that the default value for each option is true?
But that doesn't seem so, because omitting pretty option, doesn't give
output in pretty, rather pretty is used only when provided.
I find this rather confusing, it is worth documenting what is the default
value for each of these options.
Similarly, what is the expected behaviour when defaults option is not
provided,
+-- With No Owner
+SELECT ddl_filter(pg_get_database_ddl('regression_utf8', 'owner', false));
+ ddl_filter
+--------------------------------------------------------------
+ CREATE DATABASE regression_utf8 WITH CONNECTION LIMIT = 123;
+(1 row)
+
+-- With No Tablespace
+SELECT ddl_filter(pg_get_database_ddl('regression_utf8', 'defaults', true,
'tablespace', false));
+
ddl_filter
+----------------------------------------------------------------------------------------------------------------------------------------------------------
+ CREATE DATABASE regression_utf8 WITH OWNER = regress_datdba_after
ENCODING = 'UTF8' ALLOW_CONNECTIONS = true CONNECTION LIMIT = 123
IS_TEMPLATE = false;
+(1 row)
Why is connection_limit present in both of these cases...?
Another thing, what is the significance of having defaults option, because
I think that knowing non-default values could be more useful, or atleast
there should be a way to know the non-default options for the database.
Also, the option strategy is missing in the output, is it deliberate? If
yes, why?
On Thu, 5 Mar 2026 at 01:50, Akshay Joshi <[email protected]>
wrote:
> I’ve addressed all of your comments.
>
> Attached is the *v11 patch*, now ready for further review.
>
> On Wed, Mar 4, 2026 at 7:31 PM Japin Li <[email protected]> wrote:
>
>> On Wed, 04 Mar 2026 at 18:29, Akshay Joshi <[email protected]>
>> wrote:
>> > Thanks for the review, Japin. I’ve addressed all of your comments. I
>> also added a check to throw an error if an option
>> > appears more than once.
>> >
>> > Attached is the v10 patch, now ready for further review.
>> >
>>
>> Thanks for updating the patch. Here are some comments on v10.
>>
>> 1.
>> + * db_oid - OID/Name of the database for which to generate the DDL.
>>
>> Should the comment be updated? The code only accepts an OID for `db_oid`,
>> database names are not supported.
>>
>> 2.
>> + /* Set the OWNER in the DDL if owner is not omitted */
>> + if (OidIsValid(dbform->datdba) && !(ddl_flags & PG_DDL_NO_OWNER))
>> + {
>> + get_formatted_string(&buf, pretty_flags, 8, "OWNER = %s",
>> +
>> quote_identifier(dbowner));
>> + }
>>
>> `dbowner` is only needed inside this `if` — how about declaring it there
>> to
>> reduce its scope?
>>
>> 3.
>> + /* If is_with_defaults is true, then we skip default
>> encoding check */
>> + if (is_with_defaults ||
>> +
>> (pg_strcasecmp(pg_encoding_to_char(dbform->encoding),
>> +
>> DDL_DEFAULTS.DATABASE.ENCODING) != 0))
>> + {
>> + get_formatted_string(&buf, pretty_flags, 8,
>> "ENCODING = %s",
>> +
>> quote_literal_cstr(
>> +
>> pg_encoding_to_char(dbform->encoding)));
>> + }
>>
>> How about cache the result of `pg_encoding_to_char()` in a local variable
>> to
>> avoid calling it twice?
>>
>> --
>> Regards,
>> Japin Li
>> ChengDu WenWu Information Technology Co., Ltd.
>>
>
--
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH
