Hi
Date: Fri, 09 Jan 2026 13:58:09 +0800 On Thu, 08 Jan 2026 at 09:19, Mario González Troncoso <[email protected]> wrote: > On Wed, 7 Jan 2026 at 22:40, Japin Li <[email protected]> wrote: >> >> > >> > 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. >> > >> >> Yeah, you read my mind. > > Cool. I rebased this morning and it passed just fine. > Thanks for updating the patch. Here are some more comments. 1. Why not handle the [IN] ROLE and ADMIN clauses? Is there something I missed? 2. + * Returns NIL if the role OID is invalid. This can happen if the role was + * dropped concurrently, or if we're passed a OID that doesn't match + * any role. However, when I tested concurrent DROP ROLE, the function can still return a non-NIL result (though incomplete). Here’s a reproducible scenario: a) Prepare -- Session 1 CREATE USER u01 WITH CONNECTION LIMIT 10; ALTER USER u01 IN DATABASE postgres SET work_mem TO '16MB'; SELECT pg_get_role_ddl_statements('u01'::regrole); b) Set a breakpoint in Session 1's backend using GDB at pg_get_role_ddl_internal. c) Execute the query in Session 1: --- Session 1 SELECT pg_get_role_ddl_statements('u01'::regrole); d) In GDB, step over the line: tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid)); e) In Session 2, drop the user: --- Session 2 DROP USER u01; f) Continue execution in GDB. Result in Session 1: postgres=# SELECT pg_get_role_ddl_statements('u01'::regrole); pg_get_role_ddl_statements ------------------------------------------------------------------------------------------------------------------ CREATE ROLE u01 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT 10; (1 row) We only get the CREATE ROLE statement; the ALTER ROLE ... SET work_mem statement is missing. This behavior does not fully match the comment, which implies that an invalid OID would return NIL. In this case, we get a partial (and potentially misleading) result instead. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
