Re: Major Version Upgrade failure due to orphan roles entries in catalog - Mailing list pgsql-bugs

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Tue, 2025-02-11 at 15:32 +0530, Virender Singla wrote:
>> /* Postgres version:: PostgreSQL 16.6 */
>> /* The same can be reproduced in version 17 as well */
>>
>> create role my_group;
>> create role dropped_member;
>> begin;
>> grant my_group to dropped_member;
>> OTHER SESSION: drop role dropped_member;
>> BACK IN ORIGINAL SESSION:
>> commit;

[ leaves a dangling pg_auth_members entry behind ]

> I guess the GRANT statement should put a FOR KEY SHARE lock on the pg_authid row
> for "dropped_member", similar to what we do for foreign keys.

There is a lock taken already, which is why the DROP ROLE blocks.
What there is not is a recheck that the role still exists once
we have its lock.

I poked into this, and in code terms it seems like the problem is
that AddRoleMems adds a pg_auth_members entry but makes it depend
on only one of the three roles listed in the entry.  That seems
wrong on its face.  The connection to this problem is that the
dependency-adding code would take care of the lock+recheck, if
it only knew that the pg_auth_members entry ought to be treated
as depending on dropped_member.

The code the attached patch is touching is from commit 6566133c5.
I'm not going to blame the bug on that commit, because before that
we had dependencies on *zero* of the three roles; at least it added
one on the grantor.  But I'm wondering whether Robert thought about
the other two roles and explicitly rejected making dependencies
for them, and if so why.

In addition to fixing the described race condition, this patch
means that DROP OWNED BY on either the granted role or the member
will drop the grant.  This seems consistent with the goals of
6566133c5 and with our general approach to dropping privileges
during DROP OWNED BY; but it didn't happen that way before.

I'm wondering a little bit if we could simplify/remove some of the
code in user.c by relying on processing of these dependency entries
to carry the load instead during DROP ROLE.  But it passes check-world
as-is, so maybe leaving well enough alone is best.

            regards, tom lane

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 */