Re: [PATCH] Add pg_get_role_ddl() functions for role recreation

On Tue, 6 Jan 2026 at 03:27, Japin Li <japinli(at)hotmail(dot)com> 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