On Wed, Nov 19, 2025 at 3:48 PM Japin Li <[email protected]> wrote:
> > Hi Akshay, > > Thanks for updating the patch. > > On Tue, 18 Nov 2025 at 13:33, Akshay Joshi <[email protected]> > wrote: > > Hi Chao > > > > Thanks for reviewing my patch. > > > > On Tue, Nov 18, 2025 at 5:59 AM Chao Li <[email protected]> wrote: > > > > Hi Akshay, > > > > I just reviewed v3 and got some comments: > > > > > On Nov 17, 2025, at 22:34, Akshay Joshi < > [email protected]> 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*, and reconstructing the *OID* serves no practical purpose. If the majority agrees, I can extend the DDL to include OID. > > 2. > We can restrict the scope of the dbTablespace variable as follows: > > + if (OidIsValid(dbForm->dattablespace)) > + { > + char *dbTablespace = > get_tablespace_name(dbForm->dattablespace); > + if (dbTablespace) > + get_formatted_string(&buf, prettyFlags, 2, > "TABLESPACE = %s", > + > quote_identifier(dbTablespace)); > + } > Will fix this in the next patch. > > > > > > > 7 - For those parameters that have default values should be omitted. > For > > > example: > > > ``` > > > evantest=> select pg_get_database_ddl('evantest', true); > > > pg_get_database_ddl > > > ------------------------------------ > > > CREATE DATABASE evantest + > > > WITH + > > > OWNER = chaol + > > > ENCODING = "UTF8" + > > > LC_COLLATE = "en_US.UTF-8"+ > > > LC_CTYPE = "en_US.UTF-8" + > > > LOCALE_PROVIDER = 'libc' + > > > TABLESPACE = pg_default + > > > ALLOW_CONNECTIONS = true + > > > CONNECTION LIMIT = -1; > > > (1 row) > > > ``` > > > > > > I created the database “evantest” without providing any parameter. I > think > > > at least OWNER, TABLESPACE and ALLOW_CNONNECTIONS should be omitted. > For > > > CONNECTION LIMIT, you already have a logic to omit it but the logic has > > > some problem as comment 1. > > > > > > > > > > > IMHO, parameters with default values *should not* be omitted from the > > output of the pg_get_xxx_ddl functions. The primary purpose of these > > functions is to accurately reconstruct the DDL. Including all parameters > > ensures clarity, as not everyone is familiar with the default value of > > every single parameter. > > +1 > > -- > Regards, > Japin Li > ChengDu WenWu Information Technology Co., Ltd. >
