public inbox for [email protected]  

help / color / mirror / Atom feed

From: Japin Li <[email protected]>
To: Akshay Joshi <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Euler Taveira <[email protected]>
Cc: Amul Sul <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Quan Zongliang <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement
Date: Wed, 04 Mar 2026 22:01:19 +0800
Message-ID: <SY7PR01MB10921B6F9D478C3F1B2639901B67CA@SY7PR01MB10921.ausprd01.prod.outlook.com> (raw)
In-Reply-To: <CANxoLDcW2iyOq8JJYQXkzXkK6PtQX-z7Hxe=Ri3nQJX6AaPLjw@mail.gmail.com>
References: <SY7PR01MB10921BB04879E2BDE787AA5EFB67CA@SY7PR01MB10921.ausprd01.prod.outlook.com>
	<[email protected]>
	<CANxoLDcW2iyOq8JJYQXkzXkK6PtQX-z7Hxe=Ri3nQJX6AaPLjw@mail.gmail.com>

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.

view thread (41+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement
  In-Reply-To: <SY7PR01MB10921B6F9D478C3F1B2639901B67CA@SY7PR01MB10921.ausprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox