On Thu, Mar 5, 2026 at 8:15 PM Rafia Sabih <[email protected]>
wrote:
> 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
>
Following the postgresql documentation, I created ddl_defaults.h to
handle default options. This will be used across all database objects in
future pg_get_<object>_ddl patches.
> +-- 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.
>
In my view, every database has an owner; it's a required attribute, not
an optional one with a "default" value. The OWNER clause is correctly
controlled only by the PG_DDL_NO_OWNER flag. Users must explicitly
provide (*'owner',
false/'no'/'off')* to opt out of including ownership info in the DDL. When
comparing the Owner and Pretty options: by default, the Owner is included
in the DDL reconstruction, whereas the Pretty option is set to false by
default.
In my initial patches, I used flags like --no-owner and --no-tablespace
because they are very intuitive for users. However, reviewers noted that
this style is unique to pg_dump and not used elsewhere. Consequently, I
switched to the existing name-value pair format using VARIADIC arguments.
>
> 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.
>
The defaults option is false by default; if a user does not specify it,
the reconstructed DDL only includes non-default parameters. In my opinion,
we should retain this flag for users who wish to see all available
parameters for easier manual editing later. While simple database objects
have fewer parameters, this flag is essential for complex TABLE or FUNCTION
syntax where the parameter list is extensive.
> 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
>
