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

From: Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
To: Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
Cc: Japin Li <japinli(at)hotmail(dot)com>, Álvaro Herrera <alvherre(at)kurilemu(dot)de>, Euler Taveira <euler(at)eulerto(dot)com>, Amul Sul <sulamul(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, Quan Zongliang <quanzongliang(at)yeah(dot)net>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement
Date: 2026-03-05 14:45:10
Message-ID: CA+FpmFeMfmRx3fNE97X5VZpLpiausHCKsRG5aEosfTwq_idPNg@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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 <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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Euler Taveira 2026-03-05 14:48:57 Re: [Proposal] Adding Log File Capability to pg_createsubscriber
Previous Message Alexandre Felipe 2026-03-05 13:47:10 Re: index prefetching