| From: | Japin Li <japinli(at)hotmail(dot)com> |
|---|---|
| To: | Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com> |
| Cc: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, Álvaro Herrera <alvherre(at)kurilemu(dot)de>, 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: | 2025-11-20 01:39:36 |
| Message-ID: | SY0P300MB15390D9F748A65404A66555FB6D4A@SY0P300MB1539.AUSP300.PROD.OUTLOOK.COM |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Wed, 19 Nov 2025 at 16:36, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
> On Wed, Nov 19, 2025 at 3:48 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>
> Hi Akshay,
>
> Thanks for updating the patch.
>
> On Tue, 18 Nov 2025 at 13:33, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
> > Hi Chao
> >
> > Thanks for reviewing my patch.
> >
> > On Tue, Nov 18, 2025 at 5:59 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> >
> > Hi Akshay,
> >
> > I just reviewed v3 and got some comments:
> >
> > > On Nov 17, 2025, at 22:34, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
> > >
> > > All the review comments have been addressed in v3 patch.
> >
> > 1 - ruleutils.c
> > ```
> > + if (dbForm->datconnlimit != 0)
> > + get_formatted_string(&buf, prettyFlags, 1, "CONNECTION LIMIT = %d",
> > + dbForm->datconnlimit);
> > ```
> >
> > I think this is wrong. Default value of CONNECTION_LIMIT is -1 rather than 0. 0 means no connection is allowed,
> users
> > should intentionally set the value, thus 0 should be printed.
> >
> > 2 - ruleutils.c
> > ```
> > + if (!attrIsNull)
> > + get_formatted_string(&buf, prettyFlags, 1, "ICU_RULES = %s",
> > + quote_identifier(TextDatumGetCString(dbValue)));
> > ```
> >
> > ICU_RULES should be omitted if provider is not icu.
> >
> > 3 - ruleutils.c
> > ```
> > + if (!HeapTupleIsValid(tupleDatabase))
> > + ereport(ERROR,
> > + errcode(ERRCODE_UNDEFINED_OBJECT),
> > + errmsg("database with oid %d does not exist", dbOid));
> > ```
> >
> > I believe all existing code use %u to format oid. I ever raised the same comment to the other get_xxx_ddl patch.
> >
> > Fixed all above in the attached v4 patch.
>
> 1.
> Since the STRATEGY and OID parameters are not being reconstructed, should we
> document this behavior?
>
> postgres=# CREATE DATABASE mydb WITH STRATEGY file_copy OID 19876 IS_TEMPLATE true;
> CREATE DATABASE
> postgres=# SELECT pg_get_database_ddl('mydb', true);
> pg_get_database_ddl
> --------------------------------------------
> CREATE DATABASE mydb +
> WITH OWNER = japin +
> ENCODING = "UTF8" +
> LC_COLLATE = "en_US.UTF-8"+
> LC_CTYPE = "en_US.UTF-8" +
> COLLATION_VERSION = "2.39"+
> LOCALE_PROVIDER = 'libc' +
> TABLESPACE = pg_default +
> ALLOW_CONNECTIONS = true +
> CONNECTION LIMIT = -1 +
> IS_TEMPLATE = true;
> (1 row)
>
> The FormData_pg_database structure does not expose the database STRATEGY,
Yes, it's not exposed.
> and reconstructing the OID serves no practical
> purpose. If the majority agrees, I can extend the DDL to include OID.
I'm not insisting on reconstructing those parameters; I mean, we can provide
a sentence to describe this behavior.
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Smith | 2025-11-20 01:41:50 | Re: CREATE/ALTER PUBLICATION improvements for syntax synopsis |
| Previous Message | Sami Imseih | 2025-11-20 01:34:19 | Re: Add mode column to pg_stat_progress_vacuum |