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. > > 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. > I wasn't entirely sure, but after reviewing the *pg_database_size*() function, I've concluded that its usage extends beyond the Superuser and Database Owner. Specifically, other roles can view the database size if they have the *CONNECT* privilege or are Members of the *pg_read_all_stats* role. > > 5 - as you can see from the above test output, “+” in the end of every > line is weird. > The plus sign (+) is merely an artifact of *psql's* output formatting when a result cell contains a newline character (\n). It serves as a visual cue to the user that the data continues on the next line. This is confirmed by the absence of the + sign when viewing the same data in a different client, such as *pgAdmin*." To suppress this visual cue in psql, you can use the command: \pset format unaligned > > 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. > Fixed in the v4 patch and followed the docs. > > 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. > > 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. > Yes, all pg_get_xxx_ddl functions are part of a larger project, and different authors have implemented output formatting in different ways; some implementations may lack formatting altogether. Yes I agree one common function should be developed and committed so that all other authors can reuse it, ensuring consistency across the entire suite of DDL functions." > > Best regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > > > >
v4-0001-Add-pg_get_database_ddl-function-to-reconstruct-CREATE.patch
Description: Binary data
