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. 4 - ruleutils.c ``` + /* + * User must have connect privilege for target database. + */ + aclresult = object_aclcheck(DatabaseRelationId, dbOid, GetUserId(), + ACL_CONNECT); + if (aclresult != ACLCHECK_OK) + { + aclcheck_error(aclresult, OBJECT_DATABASE, + get_database_name(dbOid)); + } ``` I don’t think CONNECT privilege is good enough. By default, a new user gets CONNECT privilege via the PUBLIC role. I just did a quick test to confirm that. ``` # Create a new cluster % initdb . % pg_ctl -D . start % createdb evantest % createdb evan # connect to the db % psql -d evantest -U evan psql (19devel) Type "help" for help. # Got into the database successfully # Without any privilege grant, the user can get ddl of the system database, which seems not good evantest=> select pg_get_database_ddl('postgres', true); pg_get_database_ddl ------------------------------------ CREATE DATABASE postgres + 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) ``` IMO, only super user and database owner should be to get ddl of the database. 5 - as you can see from the above test output, “+” in the end of every line is weird. 6 - “WITH” has the same indent as the parameters, which doesn’t good look. If we look at the doc https://www.postgresql.org/docs/18/sql-createdatabase.html, “WITH” takes the first level of indent, and parameters take the second level of indent. 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. 8 - ruleutils.c ``` +/* + * get_formatted_string + * + * Return a formatted version of the string. + * + * prettyFlags - Based on prettyFlags the output includes tabs (\t) and + * newlines (\n). + * nTabChars - indent with specified number of tab characters. + * fmt - printf-style format string used by appendStringInfoVA. + */ +static void +get_formatted_string(StringInfo buf, int prettyFlags, int nTabChars, const char *fmt,...) ``` I don’t feel good with this function, but not because of the implementation of the function. I have reviewed a bunch of get_xxx_ddl patches submitted by different persons. All of them are under a big project, however, looks like to me that all authors work independently without properly coordinated. A function like this one should be common for all those patches. Maybe Alvaro can help here, pushing a common function that is used to format DDL and requesting all patches to use the function. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
