Hi, Mario González Troncoso
On Mon, 05 Jan 2026 at 07:29, Mario González Troncoso <[email protected]> wrote: > On Mon, 5 Jan 2026 at 07:17, Mario González Troncoso > <[email protected]> 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.
