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

Hi, Mario González Troncoso

On Mon, 05 Jan 2026 at 07:29, Mario González Troncoso <gonzalemario@gmail.com> wrote:
> On Mon, 5 Jan 2026 at 07:17, Mario González Troncoso
> <gonzalemario@gmail.com> wrote:
>>
>> > I reviewed your patch and found one potential issue, please check it.
>> > In pg_get_role_ddl_internal, the variable rolname is assigned from
>> > NameStr(roleform->rolname) (line 588), which means it points
>> > directly into the tuple returned from pg_authid.  After
>> > constructing the initial CREATE ROLE statement, the code calls
>> > ReleaseSysCache(tuple); (line 665), so the memory holding that
>> > NameData now belongs to the cache again. However, the function
>> > continues to use rolname when building the subsequent ALTER ROLE
>> > statements (lines 756–765).  Because the tuple has already been
>> > released, rolname is a dangling pointer and we risk reading
>> > garbage or crashing later. To fix this, copy the role name before
>> > releasing the syscache, e.g. rolname =
>> > pstrdup(NameStr(roleform->rolname));, and free it at the end.
>> >
>>
>> Good catch, I didn't know NameStr returned a pointer, for some reason
>> I've assumed I was working with a copy. Attaching the patch with the
>> changes: (also I added you in "Reviewed-by")
>
> Hi, I'm reattaching after rebasing from master and fixing a conflict on:
>
>  +++ b/src/test/regress/parallel_schedule
>  @@ -125,6 +125,10 @@ test: plancache limit plpgsql copy2 temp domain
> rangefuncs prepare conversion tr
>   # ----------
> - test: partition_join partition_prune reloptions hash_part indexing
> partition_aggregate partition_info tuplesort explain compression
> compression_lz4 memoize stats predicate numa eager_aggregate
> + test: partition_merge partition_split partition_join partition_prune
> reloptions hash_part indexing partition_aggregate partition_info
> tuplesort explain compression compression_lz4 memoize stats predicate
> numa eager_aggregate
>
> https://cirrus-ci.com/build/6142120160919552
>
> Thanks.
>
>
>> diff --git a/src/backend/utils/adt/ruleutils.c
>> b/src/backend/utils/adt/ruleutils.c
>> index 584438d05ad..41db9f10f5d 100644
>> --- a/src/backend/utils/adt/ruleutils.c
>> +++ b/src/backend/utils/adt/ruleutils.c
>> @@ -585,7 +585,7 @@ pg_get_role_ddl_internal(Oid roleid)
>>                 return NIL;
>>
>>         roleform = (Form_pg_authid) GETSTRUCT(tuple);
>> -       rolname = NameStr(roleform->rolname);
>> +       rolname = pstrdup(NameStr(roleform->rolname));
>>
>>         /*
>>          * We don't support generating DDL for system roles.  The primary reason
>> @@ -777,6 +777,7 @@ pg_get_role_ddl_internal(Oid roleid)
>>         table_close(rel, AccessShareLock);
>>
>
>

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().

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.