Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement

I have updated the documentation.
Attached is the *v12 patch*, now ready for further review.

On Fri, Mar 6, 2026 at 1:07 PM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
wrote:

>
>
> On Thu, 5 Mar 2026 at 08:16, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
> wrote:
>
>>
>>
>> On Thu, Mar 5, 2026 at 8:15 PM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
>> 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.
>>
>> Right, I think this needs to be added in the documentation as part of
> this patch. Basically, here
> <para>
> + <literal>'pretty', true</literal> (or <literal>'pretty',
> 'yes'</literal>):
> + Adds newlines and indentation for better readability.
> + </para>
> it needs to have a line more saying, by default this option is off.
> Similarly for other options, like for the owner it is by default true, etc.
>
>> 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.
>>
> This information should also be included in the documentation.
>
>>
>>
>>> 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 <akshay(dot)joshi(at)enterprisedb(dot)com>
>>> 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 <japinli(at)hotmail(dot)com> wrote:
>>>>
>>>>> On Wed, 04 Mar 2026 at 18:29, Akshay Joshi <
>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> 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
>>>
>>
>
> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>