Обсуждение: [PATCH] Add pg_get_role_ddl() functions for role recreation
Attached is a patch adding two new functions for generating DDL to
recreate roles: pg_get_role_ddl() and pg_get_role_ddl_statements().
These functions return the CREATE ROLE statement and any ALTER ROLE SET
configuration parameters needed to recreate a role. The former returns
everything as a single text string, while the latter returns each
statement as a separate row for easier programmatic processing.
The main use case is dumping role definitions for migration or backup
purposes without needing pg_dumpall. The functions handle all role
attributes (LOGIN, SUPERUSER, etc.) and both role-wide and
database-specific configuration parameters.
We intentionally don't include passwords, since we can only see the
hashed values. System roles (names starting with "pg_") are rejected
with an error, as users shouldn't be recreating those anyway.
To test:
CREATE ROLE testrole LOGIN CREATEDB CONNECTION LIMIT 5;
ALTER ROLE testrole SET work_mem TO '64MB';
SELECT pg_get_role_ddl('testrole');
Should produce:
CREATE ROLE testrole LOGIN NOSUPERUSER CREATEDB NOCREATEROLE INHERIT
NOREPLICATION NOBYPASSRLS CONNECTION LIMIT 5;
ALTER ROLE testrole SET work_mem TO '64MB';
The patch includes regression tests covering various role configurations.
Co-authored-by: Mario Gonzalez and Bryan Green.
Comments?
BG
Вложения
On 10/25/25 4:03 AM, Bryan Green wrote:
> Attached is a patch adding two new functions for generating DDL to
> recreate roles: pg_get_role_ddl() and pg_get_role_ddl_statements().
>
It is no longer apply to the latest code. Could you rebase this?
> These functions return the CREATE ROLE statement and any ALTER ROLE SET
> configuration parameters needed to recreate a role. The former returns
> everything as a single text string, while the latter returns each
> statement as a separate row for easier programmatic processing.
>
> The main use case is dumping role definitions for migration or backup
> purposes without needing pg_dumpall. The functions handle all role
> attributes (LOGIN, SUPERUSER, etc.) and both role-wide and
> database-specific configuration parameters.
>
> We intentionally don't include passwords, since we can only see the
> hashed values. System roles (names starting with "pg_") are rejected
> with an error, as users shouldn't be recreating those anyway.
>
> To test:
>
> CREATE ROLE testrole LOGIN CREATEDB CONNECTION LIMIT 5;
> ALTER ROLE testrole SET work_mem TO '64MB';
> SELECT pg_get_role_ddl('testrole');
>
> Should produce:
>
> CREATE ROLE testrole LOGIN NOSUPERUSER CREATEDB NOCREATEROLE INHERIT
> NOREPLICATION NOBYPASSRLS CONNECTION LIMIT 5;
> ALTER ROLE testrole SET work_mem TO '64MB';
>
> The patch includes regression tests covering various role configurations.
>
> Co-authored-by: Mario Gonzalez and Bryan Green.
>
> Comments?
>
> BG
On 11/6/2025 1:20 AM, Quan Zongliang wrote:
>
>
> On 10/25/25 4:03 AM, Bryan Green wrote:
>> Attached is a patch adding two new functions for generating DDL to
>> recreate roles: pg_get_role_ddl() and pg_get_role_ddl_statements().
>>
> It is no longer apply to the latest code. Could you rebase this?
>
>> These functions return the CREATE ROLE statement and any ALTER ROLE SET
>> configuration parameters needed to recreate a role. The former returns
>> everything as a single text string, while the latter returns each
>> statement as a separate row for easier programmatic processing.
>>
>> The main use case is dumping role definitions for migration or backup
>> purposes without needing pg_dumpall. The functions handle all role
>> attributes (LOGIN, SUPERUSER, etc.) and both role-wide and
>> database-specific configuration parameters.
>>
>> We intentionally don't include passwords, since we can only see the
>> hashed values. System roles (names starting with "pg_") are rejected
>> with an error, as users shouldn't be recreating those anyway.
>>
>> To test:
>>
>> CREATE ROLE testrole LOGIN CREATEDB CONNECTION LIMIT 5;
>> ALTER ROLE testrole SET work_mem TO '64MB';
>> SELECT pg_get_role_ddl('testrole');
>>
>> Should produce:
>>
>> CREATE ROLE testrole LOGIN NOSUPERUSER CREATEDB NOCREATEROLE
>> INHERIT NOREPLICATION NOBYPASSRLS CONNECTION LIMIT 5;
>> ALTER ROLE testrole SET work_mem TO '64MB';
>>
>> The patch includes regression tests covering various role configurations.
>>
>> Co-authored-by: Mario Gonzalez and Bryan Green.
>>
>> Comments?
>>
>> BG
>
The rebased patch is attached.
Thanks,
--
Bryan Green
EDB: https://www.enterprisedb.com
Вложения
On 11/7/25 12:20 AM, Bryan Green wrote:
>>
> The rebased patch is attached.
>
> Thanks,
>
Currently, we have the "CREATE USER" command. Should we consider
outputting users with the "LOGIN" attribute as "CREATE USER"?
Otherwise, it might look a little strange.
postgres=# CREATE USER testuser;
CREATE ROLE
postgres=# SELECT pg_get_role_ddl('testuser');
pg_get_role_ddl
---------------------------------------------------------------------------------------------------
CREATE ROLE testuser LOGIN NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT
NOREPLICATION NOBYPASSRLS;
(1 row)
The drawback of doing this is that the command "CREATE ROLE xxx LOGIN"
will be converted to "CREATE USER". But I still think this is better.
Regards,
--
Quan Zongliang
Hello Bryan,
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
itpoints directly into the tuple returned from pg_authid. After constructing the initial CREATE ROLE statement, the
codecalls 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).
Becausethe 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.
BR,
Yuan Li (Carol)
-----邮件原件-----
发件人: Bryan Green <dbryan.green@gmail.com>
发送时间: 2025年11月7日 0:21
收件人: Quan Zongliang <quanzongliang@yeah.net>; pgsql-hackers@lists.postgresql.org
主题: Re: [PATCH] Add pg_get_role_ddl() functions for role recreation
On 11/6/2025 1:20 AM, Quan Zongliang wrote:
>
>
> On 10/25/25 4:03 AM, Bryan Green wrote:
>> Attached is a patch adding two new functions for generating DDL to
>> recreate roles: pg_get_role_ddl() and pg_get_role_ddl_statements().
>>
> It is no longer apply to the latest code. Could you rebase this?
>
>> These functions return the CREATE ROLE statement and any ALTER ROLE
>> SET configuration parameters needed to recreate a role. The former
>> returns everything as a single text string, while the latter returns
>> each statement as a separate row for easier programmatic processing.
>>
>> The main use case is dumping role definitions for migration or backup
>> purposes without needing pg_dumpall. The functions handle all role
>> attributes (LOGIN, SUPERUSER, etc.) and both role-wide and
>> database-specific configuration parameters.
>>
>> We intentionally don't include passwords, since we can only see the
>> hashed values. System roles (names starting with "pg_") are rejected
>> with an error, as users shouldn't be recreating those anyway.
>>
>> To test:
>>
>> CREATE ROLE testrole LOGIN CREATEDB CONNECTION LIMIT 5;
>> ALTER ROLE testrole SET work_mem TO '64MB';
>> SELECT pg_get_role_ddl('testrole');
>>
>> Should produce:
>>
>> CREATE ROLE testrole LOGIN NOSUPERUSER CREATEDB NOCREATEROLE
>> INHERIT NOREPLICATION NOBYPASSRLS CONNECTION LIMIT 5;
>> ALTER ROLE testrole SET work_mem TO '64MB';
>>
>> The patch includes regression tests covering various role configurations.
>>
>> Co-authored-by: Mario Gonzalez and Bryan Green.
>>
>> Comments?
>>
>> BG
>
The rebased patch is attached.
Thanks,
--
Bryan Green
EDB: https://www.enterprisedb.com
On Fri, 7 Nov 2025 at 02:43, li carol <carol.li2025@outlook.com> wrote:
>
> Hello Bryan,
>
> 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
itpoints directly into the tuple returned from pg_authid. After constructing the initial CREATE ROLE statement, the
codecalls 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).
Becausethe 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")
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);
pfree(buf.data);
+ pfree(rolname);
return statements;
https://cirrus-ci.com/build/4813271540170752
> BR,
> Yuan Li (Carol)
>
>
[...]
> >>
> >> Co-authored-by: Mario Gonzalez and Bryan Green.
> >>
> >> Comments?
> >>
> >> BG
> >
> The rebased patch is attached.
>
> Thanks,
>
> --
> Bryan Green
> EDB: https://www.enterprisedb.com
--
Mario Gonzalez
EDB: https://www.enterprisedb.com
Вложения
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 meansit points directly into the tuple returned from pg_authid. After constructing the initial CREATE ROLE statement, thecode 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). Becausethe 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); > -- Mario Gonzalez EDB: https://www.enterprisedb.com
Вложения
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.
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
On Wed, 07 Jan 2026 at 16:28, Mario González Troncoso <gonzalemario@gmail.com> wrote: > 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. > Yeah, you read my mind. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
On Wed, 7 Jan 2026 at 22:40, Japin Li <japinli@hotmail.com> 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. > > -- > Regards, > Japin Li > ChengDu WenWu Information Technology Co., Ltd. -- Mario Gonzalez EDB: https://www.enterprisedb.com
Вложения
Hi Date: Fri, 09 Jan 2026 13:58:09 +0800 On Thu, 08 Jan 2026 at 09:19, Mario González Troncoso <gonzalemario@gmail.com> wrote: > On Wed, 7 Jan 2026 at 22:40, Japin Li <japinli@hotmail.com> 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.