On Tue, 6 Jan 2026 at 03:27, Japin Li <[email protected]> wrote: > [...] > > Thanks for updating the patch. Some comments on v4 > > 1. > > + const char *separator = " "; > + > ... > + appendStringInfo(&buf, "%s%s", separator, > + roleform->rolcanlogin ? "LOGIN" : > "NOLOGIN"); > + > > The separator is never changed in pg_get_role_ddl_internal(), so we can remove > the variable and hard-code it in appendStringInfo(). >
Is that what you mean by "remove the variable and hard-code"? @@ -578,7 +578,6 @@ pg_get_role_ddl_internal(Oid roleid) - const char *separator = " "; tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid)); if (!HeapTupleIsValid(tuple)) @@ -605,34 +604,34 @@ pg_get_role_ddl_internal(Oid roleid) * you'd typically write them in a CREATE ROLE command, though any order * is actually acceptable to the parser. */ - appendStringInfo(&buf, "%s%s", separator, - roleform->rolcanlogin ? "LOGIN" : "NOLOGIN"); - - appendStringInfo(&buf, "%s%s", separator, + appendStringInfo(&buf, " %s", The lines above are a snippet of the latest commit `WIP: removing "separator"` on https://cirrus-ci.com/build/4621719253549056 Would you be able to see the whole change over there? If that's what you mean, I'll squash afterwards and attach a new patch version to this thread. I applied the other feedback about foreach_ptr and to preserve the order as in the docs, thanks! > 2. > + appendStringInfo(&buf, "%s%s", separator, > + roleform->rolcanlogin ? "LOGIN" : > "NOLOGIN"); > + > + appendStringInfo(&buf, "%s%s", separator, > + roleform->rolsuper ? "SUPERUSER" : > "NOSUPERUSER"); > + > + appendStringInfo(&buf, "%s%s", separator, > + roleform->rolcreatedb ? "CREATEDB" : > "NOCREATEDB"); > + > + appendStringInfo(&buf, "%s%s", separator, > + roleform->rolcreaterole ? > "CREATEROLE" : "NOCREATEROLE"); > + > + appendStringInfo(&buf, "%s%s", separator, > + roleform->rolinherit ? "INHERIT" : > "NOINHERIT"); > + > + appendStringInfo(&buf, "%s%s", separator, > + roleform->rolreplication ? > "REPLICATION" : "NOREPLICATION"); > + > + appendStringInfo(&buf, "%s%s", separator, > + roleform->rolbypassrls ? "BYPASSRLS" > : "NOBYPASSRLS"); > > For these options, I suggest preserving the same order as in the > documentation. > > postgres=# \h create role > Command: CREATE ROLE > Description: define a new database role > Syntax: > CREATE ROLE name [ [ WITH ] option [ ... ] ] > > where option can be: > > SUPERUSER | NOSUPERUSER > | CREATEDB | NOCREATEDB > | CREATEROLE | NOCREATEROLE > | INHERIT | NOINHERIT > | LOGIN | NOLOGIN > | REPLICATION | NOREPLICATION > | BYPASSRLS | NOBYPASSRLS > | CONNECTION LIMIT connlimit > | [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL > | VALID UNTIL 'timestamp' > | IN ROLE role_name [, ...] > | ROLE role_name [, ...] > | ADMIN role_name [, ...] > | SYSID uid > > 3. > > + foreach(lc, statements) > + { > + char *stmt = (char *) lfirst(lc); > + > + if (!first) > + appendStringInfoChar(&result, '\n'); > + appendStringInfoString(&result, stmt); > + first = false; > + } > > The foreach() macro can be replaced by foreach_ptr(), allowing us to remove > the lc variable entirely. > > foreach_ptr(char, stmt, statements) > { > if (!first) > appendStringInfoChar(&result, '\n'); > appendStringInfoString(&result, stmt); > first = false; > } > > -- > Regards, > Japin Li > ChengDu WenWu Information Technology Co., Ltd. -- Mario Gonzalez EDB: https://www.enterprisedb.com
