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

On Tue, 6 Jan 2026 at 03:27, Japin Li <japinli@hotmail.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