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) 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)); + } > > > > 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.
