public inbox for [email protected]help / color / mirror / Atom feed
Re: Major Version Upgrade failure due to orphan roles entries in catalog 11+ messages / 4 participants [nested] [flat]
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2025-02-13 17:16 Álvaro Herrera <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: Álvaro Herrera @ 2025-02-13 17:16 UTC (permalink / raw) To: Virender Singla <[email protected]>; +Cc: [email protected]; Aniket Jha <[email protected]> On 2025-Feb-11, Virender Singla wrote: > And the upgrade fails with an error : > > > *GRANT "my_group" TO "" WITH INHERIT TRUE GRANTED BY "postgres";ERROR: > zero-length delimited identifier at or near """"* > > The issue seems to be coming from pg_dumpall for building grants during > pg_upgrade. Hmm, I think fixing the bug as Tom suggests downthread is probably a good idea, but I think we should in addition change pg_dumpall to avoid printing a GRANT line if there's no grantee. Maybe turning one of these LEFT JOINs into a regular inner join is a sufficient fix for that: /* Generate and execute query. */ printfPQExpBuffer(buf, "SELECT ur.rolname AS role, " "um.rolname AS member, " "ug.oid AS grantorid, " "ug.rolname AS grantor, " "a.admin_option"); if (dump_grant_options) appendPQExpBufferStr(buf, ", a.inherit_option, a.set_option"); appendPQExpBuffer(buf, " FROM pg_auth_members a " "LEFT JOIN %s ur on ur.oid = a.roleid " "LEFT JOIN %s um on um.oid = a.member " "LEFT JOIN %s ug on ug.oid = a.grantor " "WHERE NOT (ur.rolname ~ '^pg_' AND um.rolname ~ '^pg_')" "ORDER BY 1,2,4", role_catalog, role_catalog, role_catalog); -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "I am amazed at [the pgsql-sql] mailing list for the wonderful support, and lack of hesitasion in answering a lost soul's question, I just wished the rest of the mailing list could be like this." (Fotis) https://postgr.es/m/[email protected] ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2025-02-13 17:33 Tom Lane <[email protected]> parent: Álvaro Herrera <[email protected]> 1 sibling, 0 replies; 11+ messages in thread From: Tom Lane @ 2025-02-13 17:33 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> =?utf-8?Q?=C3=81lvaro?= Herrera <[email protected]> writes: > Hmm, I think fixing the bug as Tom suggests downthread is probably a > good idea, but I think we should in addition change pg_dumpall to avoid > printing a GRANT line if there's no grantee. On the one hand, my proposed patch can do nothing to fix existing dangling entries in pg_auth_members, so hacking pg_dump seems like a good workaround if the problem already exists. On the other hand, if we make pg_dump do that then we won't detect future problems of the same ilk. > Maybe turning one of these LEFT > JOINs into a regular inner join is a sufficient fix for that: Probably change all three, if we're to do this at all. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2025-02-20 22:19 Tom Lane <[email protected]> parent: Álvaro Herrera <[email protected]> 1 sibling, 2 replies; 11+ messages in thread From: Tom Lane @ 2025-02-20 22:19 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> =?utf-8?Q?=C3=81lvaro?= Herrera <[email protected]> writes: > Hmm, I think fixing the bug as Tom suggests downthread is probably a > good idea, but I think we should in addition change pg_dumpall to avoid > printing a GRANT line if there's no grantee. Maybe turning one of these LEFT > JOINs into a regular inner join is a sufficient fix for that: After looking at this I thought it was worth a little more code to warn about the dangling role OID, instead of just silently ignoring it. Here's a couple of more-polished patches. I'm unsure whether to back-patch the 0001 patch, as it does imply more pg_shdepend entries than we have today, so it's sort of a backdoor catalog change. But we're mostly interested in the transient behavior of having a lock+recheck during entry insertion, so maybe it's fine. 0002 should be back-patched in any case. (BTW, I was distressed to learn from the code coverage report that we have zero test coverage of the hardly-trivial logic in dumpRoleMembership. I didn't try to address that here. I did test this new logic by dint of manually deleting from pg_authid.) regards, tom lane Attachments: [text/x-diff] v1-0001-Avoid-race-condition-between-GRANT-role-and-DROP-.patch (7.5K, 2-v1-0001-Avoid-race-condition-between-GRANT-role-and-DROP-.patch) download | inline diff: From b8f0dff492cfe689e8516b0353d004f483838640 Mon Sep 17 00:00:00 2001 From: Tom Lane <[email protected]> Date: Thu, 20 Feb 2025 15:34:55 -0500 Subject: [PATCH v1 1/2] Avoid race condition between "GRANT role" and "DROP ROLE". Concurrently dropping either the granted role or the grantee does not stop GRANT from completing, instead resulting in a dangling role reference in pg_auth_members. That's relatively harmless in the short run, but it does result in garbage output from pg_dumpall and therefore problems for pg_upgrade. This patch deals with the problem by adding the granted and grantee roles as explicit shared dependencies of the pg_auth_members entry. That's a bit indirect, but it works because the pg_shdepend code applies the necessary locking and rechecking. Commit 6566133c5 previously established similar handling for the grantor column of pg_auth_members; it's not clear why it didn't cover the other two role OID columns. A side-effect of this approach is that DROP OWNED BY will now drop pg_auth_members entries that mention the target role as either the granted or grantee role. That's clearly appropriate for the grantee, and it doesn't seem too far out of line for the granted role. (One could argue that this makes DropRole's code to auto-drop pg_auth_members entries unnecessary, but I chose to leave it in place since perhaps some people's workflows expect that to work without a DROP OWNED BY.) Reported-by: Virender Singla <[email protected]> Discussion: https://postgr.es/m/CAM6Zo8woa62ZFHtMKox6a4jb8qQ=w87R2L0K8347iE-juQL2EA@mail.gmail.com Backpatch-through: TBD --- doc/src/sgml/ref/drop_owned.sgml | 2 +- src/backend/commands/user.c | 22 +++++++++++++---- src/test/regress/expected/privileges.out | 30 ++++++++++++++++++++++++ src/test/regress/sql/privileges.sql | 15 ++++++++++++ 4 files changed, 63 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/ref/drop_owned.sgml b/doc/src/sgml/ref/drop_owned.sgml index efda01a39e..46e1c229ec 100644 --- a/doc/src/sgml/ref/drop_owned.sgml +++ b/doc/src/sgml/ref/drop_owned.sgml @@ -33,7 +33,7 @@ DROP OWNED BY { <replaceable class="parameter">name</replaceable> | CURRENT_ROLE database that are owned by one of the specified roles. Any privileges granted to the given roles on objects in the current database or on shared objects (databases, tablespaces, configuration - parameters) will also be revoked. + parameters, or other roles) will also be revoked. </para> </refsect1> diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 0db174e6f1..0c84886e82 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -30,6 +30,7 @@ #include "commands/defrem.h" #include "commands/seclabel.h" #include "commands/user.h" +#include "lib/qunique.h" #include "libpq/crypt.h" #include "miscadmin.h" #include "storage/lmgr.h" @@ -489,7 +490,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt) * Advance command counter so we can see new record; else tests in * AddRoleMems may fail. */ - if (addroleto || adminmembers || rolemembers) + if (addroleto || adminmembers || rolemembers || !superuser()) CommandCounterIncrement(); /* Default grant. */ @@ -1904,7 +1905,8 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid, else { Oid objectId; - Oid *newmembers = palloc(sizeof(Oid)); + Oid *newmembers = (Oid *) palloc(3 * sizeof(Oid)); + int nnewmembers; /* * The values for these options can be taken directly from 'popt'. @@ -1946,12 +1948,22 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid, new_record, new_record_nulls); CatalogTupleInsert(pg_authmem_rel, tuple); - /* updateAclDependencies wants to pfree array inputs */ - newmembers[0] = grantorId; + /* + * Record dependencies on the roleid, member, and grantor, as if a + * pg_auth_members entry were an object ACL. + * updateAclDependencies() requires an input array that is + * palloc'd (it will free it), sorted, and de-duped. + */ + newmembers[0] = roleid; + newmembers[1] = memberid; + newmembers[2] = grantorId; + qsort(newmembers, 3, sizeof(Oid), oid_cmp); + nnewmembers = qunique(newmembers, 3, sizeof(Oid), oid_cmp); + updateAclDependencies(AuthMemRelationId, objectId, 0, InvalidOid, 0, NULL, - 1, newmembers); + nnewmembers, newmembers); } /* CCI after each change, in case there are duplicates in list */ diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 6b01313101..a76256405f 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -113,6 +113,36 @@ CREATE USER regress_priv_user2; CREATE USER regress_priv_user3; CREATE USER regress_priv_user4; CREATE USER regress_priv_user5; +-- DROP OWNED should also act on granted and granted-to roles +GRANT regress_priv_user1 TO regress_priv_user2; +GRANT regress_priv_user2 TO regress_priv_user3; +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; + roleid | member +--------------------+-------------------- + regress_priv_user1 | regress_priv_user2 + regress_priv_user2 | regress_priv_user3 +(2 rows) + +REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4; -- no effect +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; + roleid | member +--------------------+-------------------- + regress_priv_user1 | regress_priv_user2 + regress_priv_user2 | regress_priv_user3 +(2 rows) + +DROP OWNED BY regress_priv_user2; -- removes both grants +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; + roleid | member +--------+-------- +(0 rows) + GRANT pg_read_all_data TO regress_priv_user6; GRANT pg_write_all_data TO regress_priv_user7; GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION; diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index 60e7443bf5..d195aaf137 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -90,6 +90,21 @@ CREATE USER regress_priv_user3; CREATE USER regress_priv_user4; CREATE USER regress_priv_user5; +-- DROP OWNED should also act on granted and granted-to roles +GRANT regress_priv_user1 TO regress_priv_user2; +GRANT regress_priv_user2 TO regress_priv_user3; +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; +REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4; -- no effect +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; +DROP OWNED BY regress_priv_user2; -- removes both grants +SELECT roleid::regrole, member::regrole FROM pg_auth_members + WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole) + ORDER BY roleid::regrole::text; + GRANT pg_read_all_data TO regress_priv_user6; GRANT pg_write_all_data TO regress_priv_user7; GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION; -- 2.43.5 [text/x-diff] v1-0002-Fix-pg_dumpall-to-cope-with-dangling-OIDs-in-pg_a.patch (5.4K, 3-v1-0002-Fix-pg_dumpall-to-cope-with-dangling-OIDs-in-pg_a.patch) download | inline diff: From 937b1ebb7c8eeb022385c51ec13ea206bd97d2a3 Mon Sep 17 00:00:00 2001 From: Tom Lane <[email protected]> Date: Thu, 20 Feb 2025 17:07:34 -0500 Subject: [PATCH v1 2/2] Fix pg_dumpall to cope with dangling OIDs in pg_auth_members. In view of the bug fixed by the previous patch, we need to allow for the possibility of role OIDs in pg_auth_members that don't exist in pg_authid. As pg_dumpall stands, it will emit invalid commands like 'GRANT foo TO ""', which causes pg_upgrade to fail. Fix it to emit warnings and skip those GRANTs, instead. There was some discussion of removing the problem by changing dumpRoleMembership's query to use JOIN not LEFT JOIN, but that would result in silently ignoring such entries. It seems better to produce a warning. Reported-by: Virender Singla <[email protected]> Discussion: https://postgr.es/m/CAM6Zo8woa62ZFHtMKox6a4jb8qQ=w87R2L0K8347iE-juQL2EA@mail.gmail.com Backpatch-through: TBD --- src/bin/pg_dump/pg_dumpall.c | 66 +++++++++++++++++++++++++++++++----- 1 file changed, 57 insertions(+), 9 deletions(-) diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index b993b05cc2..b9b22c47be 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -986,6 +986,13 @@ dumpRoleMembership(PGconn *conn) total; bool dump_grantors; bool dump_grant_options; + int i_role; + int i_member; + int i_grantor; + int i_roleid; + int i_memberid; + int i_grantorid; + int i_admin_option; int i_inherit_option; int i_set_option; @@ -995,6 +1002,10 @@ dumpRoleMembership(PGconn *conn) * they didn't have ADMIN OPTION on the role, or a user that no longer * existed. To avoid dump and restore failures, don't dump the grantor * when talking to an old server version. + * + * Also, in older versions the roleid and/or member could be role OIDs + * that no longer exist. If we find such cases, print a warning and skip + * the entry. */ dump_grantors = (PQserverVersion(conn) >= 160000); @@ -1006,8 +1017,10 @@ dumpRoleMembership(PGconn *conn) /* Generate and execute query. */ printfPQExpBuffer(buf, "SELECT ur.rolname AS role, " "um.rolname AS member, " - "ug.oid AS grantorid, " "ug.rolname AS grantor, " + "a.roleid AS roleid, " + "a.member AS memberid, " + "a.grantor AS grantorid, " "a.admin_option"); if (dump_grant_options) appendPQExpBufferStr(buf, ", a.inherit_option, a.set_option"); @@ -1016,8 +1029,15 @@ dumpRoleMembership(PGconn *conn) "LEFT JOIN %s um on um.oid = a.member " "LEFT JOIN %s ug on ug.oid = a.grantor " "WHERE NOT (ur.rolname ~ '^pg_' AND um.rolname ~ '^pg_')" - "ORDER BY 1,2,4", role_catalog, role_catalog, role_catalog); + "ORDER BY 1,2,3", role_catalog, role_catalog, role_catalog); res = executeQuery(conn, buf->data); + i_role = PQfnumber(res, "role"); + i_member = PQfnumber(res, "member"); + i_grantor = PQfnumber(res, "grantor"); + i_roleid = PQfnumber(res, "roleid"); + i_memberid = PQfnumber(res, "memberid"); + i_grantorid = PQfnumber(res, "grantorid"); + i_admin_option = PQfnumber(res, "admin_option"); i_inherit_option = PQfnumber(res, "inherit_option"); i_set_option = PQfnumber(res, "set_option"); @@ -1041,24 +1061,32 @@ dumpRoleMembership(PGconn *conn) total = PQntuples(res); while (start < total) { - char *role = PQgetvalue(res, start, 0); + char *role = PQgetvalue(res, start, i_role); int i; bool *done; int remaining; int prev_remaining = 0; rolename_hash *ht; + /* If we hit a null roleid, we're done (nulls sort to the end). */ + if (PQgetisnull(res, start, i_role)) + { + /* translator: %s represents a numeric role OID */ + pg_log_warning("found dangling pg_auth_members entry for role %s", + PQgetvalue(res, start, i_roleid)); + break; + } + /* All memberships for a single role should be adjacent. */ for (end = start; end < total; ++end) { char *otherrole; - otherrole = PQgetvalue(res, end, 0); + otherrole = PQgetvalue(res, end, i_role); if (strcmp(role, otherrole) != 0) break; } - role = PQgetvalue(res, start, 0); remaining = end - start; done = pg_malloc0(remaining * sizeof(bool)); ht = rolename_create(remaining, NULL); @@ -1098,10 +1126,30 @@ dumpRoleMembership(PGconn *conn) if (done[i - start]) continue; - member = PQgetvalue(res, i, 1); - grantorid = PQgetvalue(res, i, 2); - grantor = PQgetvalue(res, i, 3); - admin_option = PQgetvalue(res, i, 4); + /* Complain about, then ignore, entries with dangling OIDs. */ + if (PQgetisnull(res, i, i_member)) + { + /* translator: %s represents a numeric role OID */ + pg_log_warning("found dangling pg_auth_members entry for role %s", + PQgetvalue(res, i, i_memberid)); + done[i - start] = true; + --remaining; + continue; + } + if (PQgetisnull(res, i, i_grantor)) + { + /* translator: %s represents a numeric role OID */ + pg_log_warning("found dangling pg_auth_members entry for role %s", + PQgetvalue(res, i, i_grantorid)); + done[i - start] = true; + --remaining; + continue; + } + + member = PQgetvalue(res, i, i_member); + grantor = PQgetvalue(res, i, i_grantor); + grantorid = PQgetvalue(res, i, i_grantorid); + admin_option = PQgetvalue(res, i, i_admin_option); if (dump_grant_options) set_option = PQgetvalue(res, i, i_set_option); -- 2.43.5 ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2025-02-21 07:18 Laurenz Albe <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Laurenz Albe @ 2025-02-21 07:18 UTC (permalink / raw) To: Tom Lane <[email protected]>; Álvaro Herrera <[email protected]>; +Cc: Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> On Thu, 2025-02-20 at 17:19 -0500, Tom Lane wrote: > After looking at this I thought it was worth a little more code to warn > about the dangling role OID, instead of just silently ignoring it. > Here's a couple of more-polished patches. > > I'm unsure whether to back-patch the 0001 patch, as it does imply > more pg_shdepend entries than we have today, so it's sort of a > backdoor catalog change. But we're mostly interested in the > transient behavior of having a lock+recheck during entry insertion, > so maybe it's fine. 0002 should be back-patched in any case. I'd say that adding new catalog entries in a way that is compatible shouldn't be a problem, but I still wouldn't backpatch the 0001 patch, because it is not necessary. The orphaned pg_auth_members entry didn't cause any harm, and a few warnings more during an upgrade shouldn't be a big problem. I have one question about the first patch: > diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c > index 0db174e6f1..0c84886e82 100644 > --- a/src/backend/commands/user.c > +++ b/src/backend/commands/user.c > @@ -489,7 +490,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt) > * Advance command counter so we can see new record; else tests in > * AddRoleMems may fail. > */ > - if (addroleto || adminmembers || rolemembers) > + if (addroleto || adminmembers || rolemembers || !superuser()) > CommandCounterIncrement(); > > /* Default grant. */ That change seems unrelated to the problem at hand, and I don't see it mentioned in the commit message. Is that an oversight you fixed on the fly? Apart from that, the patches look fine. Yours, Laurenz Albe -- *E-Mail Disclaimer* Der Inhalt dieser E-Mail ist ausschliesslich fuer den bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen. *CONFIDENTIALITY NOTICE & DISCLAIMER *This message and any attachment are confidential and may be privileged or otherwise protected from disclosure and solely for the use of the person(s) or entity to whom it is intended. If you have received this message in error and are not the intended recipient, please notify the sender immediately and delete this message and any attachment from your system. If you are not the intended recipient, be advised that any use of this message is prohibited and may be unlawful, and you must not copy this message or attachment or disclose the contents to any other person. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2025-02-21 14:41 Tom Lane <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tom Lane @ 2025-02-21 14:41 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> Laurenz Albe <[email protected]> writes: > I have one question about the first patch: >> - if (addroleto || adminmembers || rolemembers) >> + if (addroleto || adminmembers || rolemembers || !superuser()) >> CommandCounterIncrement(); > That change seems unrelated to the problem at hand, and I don't see it > mentioned in the commit message. Is that an oversight you fixed on the > fly? Well, kinda, because the patch doesn't work without it. The problematic case is where none of those 3 flags are set and also !superuser, so that we decide we need the default grant implemented a few lines further down. That grant now has an explicit reference to the new roleid, and if we haven't CommandCounterIncrement'ed, the pg_shdepend code will error out because it doesn't see the catalog entry for roleid. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2025-02-21 16:23 Laurenz Albe <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Laurenz Albe @ 2025-02-21 16:23 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> On Fri, 2025-02-21 at 09:41 -0500, Tom Lane wrote: > Laurenz Albe <[email protected]> writes: > > I have one question about the first patch: > > > > - if (addroleto || adminmembers || rolemembers) > > > + if (addroleto || adminmembers || rolemembers || !superuser()) > > > CommandCounterIncrement(); > > > That change seems unrelated to the problem at hand, and I don't see it > > mentioned in the commit message. Is that an oversight you fixed on the > > fly? > > Well, kinda, because the patch doesn't work without it. The > problematic case is where none of those 3 flags are set and also > !superuser, so that we decide we need the default grant implemented a > few lines further down. That grant now has an explicit reference to > the new roleid, and if we haven't CommandCounterIncrement'ed, the > pg_shdepend code will error out because it doesn't see the catalog > entry for roleid. Thanks for the explanation. That might be worth a comment. Yours, Laurenz Albe -- *E-Mail Disclaimer* Der Inhalt dieser E-Mail ist ausschliesslich fuer den bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen. *CONFIDENTIALITY NOTICE & DISCLAIMER *This message and any attachment are confidential and may be privileged or otherwise protected from disclosure and solely for the use of the person(s) or entity to whom it is intended. If you have received this message in error and are not the intended recipient, please notify the sender immediately and delete this message and any attachment from your system. If you are not the intended recipient, be advised that any use of this message is prohibited and may be unlawful, and you must not copy this message or attachment or disclose the contents to any other person. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2025-02-21 16:31 Tom Lane <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tom Lane @ 2025-02-21 16:31 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> Laurenz Albe <[email protected]> writes: > Thanks for the explanation. That might be worth a comment. The adjacent comment already says /* * Advance command counter so we can see new record; else tests in * AddRoleMems may fail. */ so I didn't see anything to add there. Maybe "We can skip this in cases where we will not call AddRoleMems"? Or maybe the better answer is to conclude that the whole idea of not calling CommandCounterIncrement unconditionally is too fragile and not worth expending brain cells on, and just rip out the if-test. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2025-02-21 21:39 Laurenz Albe <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Laurenz Albe @ 2025-02-21 21:39 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> On Fri, 2025-02-21 at 11:31 -0500, Tom Lane wrote: > Laurenz Albe <[email protected]> writes: > > Thanks for the explanation. That might be worth a comment. > > The adjacent comment already says > > /* > * Advance command counter so we can see new record; else tests in > * AddRoleMems may fail. > */ > > so I didn't see anything to add there. Maybe "We can skip this in > cases where we will not call AddRoleMems"? Or maybe the better answer > is to conclude that the whole idea of not calling > CommandCounterIncrement unconditionally is too fragile and not worth > expending brain cells on, and just rip out the if-test. Both the extra sentence and the simplification feel like an improvement. I am fine with either. Yours, Laurenz Albe -- *E-Mail Disclaimer* Der Inhalt dieser E-Mail ist ausschliesslich fuer den bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen. *CONFIDENTIALITY NOTICE & DISCLAIMER *This message and any attachment are confidential and may be privileged or otherwise protected from disclosure and solely for the use of the person(s) or entity to whom it is intended. If you have received this message in error and are not the intended recipient, please notify the sender immediately and delete this message and any attachment from your system. If you are not the intended recipient, be advised that any use of this message is prohibited and may be unlawful, and you must not copy this message or attachment or disclose the contents to any other person. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2025-02-21 21:45 Tom Lane <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Tom Lane @ 2025-02-21 21:45 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> Laurenz Albe <[email protected]> writes: > On Fri, 2025-02-21 at 11:31 -0500, Tom Lane wrote: >> ... Or maybe the better answer >> is to conclude that the whole idea of not calling >> CommandCounterIncrement unconditionally is too fragile and not worth >> expending brain cells on, and just rip out the if-test. > Both the extra sentence and the simplification feel like an improvement. > I am fine with either. The more I think about it the more I like just getting rid of the test. It'll likely break with every future change to this logic, until somebody finally gives up on it; so why not now? I'll make it so. Thanks for reviewing! regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2026-02-25 13:59 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 11+ messages in thread From: Robert Haas @ 2026-02-25 13:59 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> On Thu, Feb 20, 2025 at 5:19 PM Tom Lane <[email protected]> wrote: > I'm unsure whether to back-patch the 0001 patch, as it does imply > more pg_shdepend entries than we have today, so it's sort of a > backdoor catalog change. But we're mostly interested in the > transient behavior of having a lock+recheck during entry insertion, > so maybe it's fine. 0002 should be back-patched in any case. I recently learned of a case where this commit caused role grants to be erroneously emitted from the output of pg_dumpall. In the case in question, a v16 pg_dumpall was used against an older server. Hence, dump_grantors was false, and any generated GRANT commands would not have included in the grantor anyway. Nevertheless, this logic caused those grants to be skipped altogether: + if (PQgetisnull(res, i, i_grantor)) + { + /* translator: %s represents a numeric role OID */ + pg_log_warning("found orphaned pg_auth_members entry for role %s", + PQgetvalue(res, i, i_grantorid)); + done[i - start] = true; + --remaining; + continue; + } I don't think this logic makes sense. In pre-16 releases, we don't even try to maintain the grantor field properly. Consider this test case: create role foo; create role bar; create role baz createrole; set role baz; grant foo to bar; reset role; drop role baz; If you do this on v15 and then run v15's pg_dumpall, it will dump "GRANT foo to bar", with no GRANTOR clause due to the PQgetisnull() gating that logic. v16's pg_dumpall will dump nothing and emit a warning instead. Arguably, pre-v16 pg_dumpall shouldn't EVER be dumping the grantor since the grantorid could be an old role OID that has been recycled for a new role, and relying on that for anything security-critical seems like a mistake, but that behavior is also longstanding. But omitting the grant altogether seems like an overreaction. I understand that we need to do that when the *member* is invalid, of course; in that case, there's no alternative. But that's not the case for the grantor. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog @ 2026-02-25 15:36 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Tom Lane @ 2026-02-25 15:36 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]> Robert Haas <[email protected]> writes: > I recently learned of a case where this commit caused role grants to > be erroneously emitted from the output of pg_dumpall. In the case in > question, a v16 pg_dumpall was used against an older server. Hence, > dump_grantors was false, and any generated GRANT commands would not > have included in the grantor anyway. Nevertheless, this logic caused > those grants to be skipped altogether: > ... > If you do this on v15 and then run v15's pg_dumpall, it will dump > "GRANT foo to bar", with no GRANTOR clause due to the PQgetisnull() > gating that logic. v16's pg_dumpall will dump nothing and emit a > warning instead. Arguably, pre-v16 pg_dumpall shouldn't EVER be > dumping the grantor since the grantorid could be an old role OID that > has been recycled for a new role, and relying on that for anything > security-critical seems like a mistake, but that behavior is also > longstanding. But omitting the grant altogether seems like an > overreaction. I understand that we need to do that when the *member* > is invalid, of course; in that case, there's no alternative. But > that's not the case for the grantor. Hmm. As per the commit message, Pre-v16 branches already coped with dangling grantor OIDs by simply omitting the GRANTED BY clause. I left that behavior as-is, although it's somewhat inconsistent with the behavior of later branches. So what you're saying is that I should have made the later branches do that also. I guess it's arguably better than dropping the grant altogether ... but the end result will be that the grant is now granted by the superuser running the restore, which doesn't seem very good either. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-02-25 15:36 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-02-13 17:16 Re: Major Version Upgrade failure due to orphan roles entries in catalog Álvaro Herrera <[email protected]> 2025-02-13 17:33 ` Tom Lane <[email protected]> 2025-02-20 22:19 ` Tom Lane <[email protected]> 2025-02-21 07:18 ` Laurenz Albe <[email protected]> 2025-02-21 14:41 ` Tom Lane <[email protected]> 2025-02-21 16:23 ` Laurenz Albe <[email protected]> 2025-02-21 16:31 ` Tom Lane <[email protected]> 2025-02-21 21:39 ` Laurenz Albe <[email protected]> 2025-02-21 21:45 ` Tom Lane <[email protected]> 2026-02-25 13:59 ` Robert Haas <[email protected]> 2026-02-25 15:36 ` Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox