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(at)gmail(dot)com> wrote:
> On Mon, 5 Jan 2026 at 07:17, Mario González Troncoso
> <gonzalemario(at)gmail(dot)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.