Subject: Fix races conditions in DropRole() and GrantRole()

From: Bertrand Drouvot <[email-masked]>
Date: Sat, 4 Jul 2026 07:47:08 +0000

Hi hackers,

While working on [1], I observed that DropRole() and GrantRole() have the same
"use stale data after the lock is acquired" issues.

Indeed, DropRole() and GrantRole() resolve the role name to an OID before acquiring
LockSharedObject() on the role. A concurrent session that commits a DROP ROLE
between the read and the lock acquisition leaves the first session acting on a
stale OID.

Examples:

1/ DROP ROLE + concurrent DROP ROLE

CREATE ROLE testrole;
gdb breakpoint at user.c:1198 (before LockSharedObject) on session 1

session 1: DROP ROLE testrole;
session 1 is paused by the breakpoint
session 2: DROP ROLE testrole;
continue session 1 produces:
ERROR: could not find tuple for role 24662

2/ GRANT ROLE + concurrent DROP ROLE

CREATE ROLE testrole;
CREATE ROLE testmember;
gdb breakpoint at user.c:1716 (before LockSharedObject) on session 1

session 1: GRANT testrole TO testmember;
session is paused by the breakpoint
session 2: DROP ROLE testrole;
continue session 1: GRANT ROLE succeeds

It produces an orphaned pg_auth_members entry:

postgres=#   SELECT m.member::regrole, m.roleid, r.rolname
  FROM pg_auth_members m
  LEFT JOIN pg_roles r ON m.roleid = r.oid
  WHERE r.oid IS NULL;
   member   | roleid | rolname
------------+--------+---------
 testmember |  16386 |

The patch attached fixes the races by using the same approach as
RangeVarGetRelidExtended(): It encapsulates name resolution, permission checking
(via a caller-supplied callback), and lock acquisition inside a retry loop driven
by SharedInvalidMessageCounter. If invalidation messages arrive between name
resolution and locking, indicating concurrent DDL, the function retries.

The lock is kept across retries and only released if the name resolves to a
different OID on the next iteration.

Two callbacks are provided:
- RoleNameCallbackForDropRole(): checks current/session user, superuser attribute,
and ADMIN OPTION privilege before locking. This is similar to what DropRole() is
currently doing before LockSharedObject().
- RoleNameCallbackForGrantRole(): calls check_role_membership_authorization() to
verify the current user can grant/revoke membership. This is similar to what GrantRole()
is currently doing before calling AddRoleMems()/DelRoleMems().

DropRole() and GrantRole() now call RoleNameGetOid() with appropriate lock
levels.

Remark:

AlterRole() does not need the fix because it calls CatalogTupleUpdate() on the
pg_authid tuple before AddRoleMems(), which blocks a concurrent DROP ROLE.

[1]: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 6 Jul 2026 13:22:39 +0000

Hi,

> resolution and locking, indicating concurrent DDL, the function retries.

Also I think we could make use of the same approach to fix one of the issue that
was discussed in [1], means:

"
create role my_group;
create role dropped_member;

Session 1: begin;grant my_group to dropped_member;
Session 2: drop role dropped_member;
Session 1: commit;
"

producing an orphaned pg_auth_members entry:

postgres=# SELECT m.member, m.roleid, r.rolname
    FROM pg_auth_members m
    LEFT JOIN pg_roles r ON m.member = r.oid
    WHERE r.oid IS NULL;
 member | roleid | rolname
--------+--------+---------
  16385 |  16384 |
(1 row)

0002 fixes this by acquiring AccessShareLock on each resolved role within
roleSpecsToIds(), ensuring the role cannot be dropped while any caller is using
its OID.

It also fixes other cases that would produce orphaned pg_auth_members entries,
like "ALTER GROUP my_group ADD USER dropped_member" or
"CREATE ROLE new_group ROLE dropped_member" with a concurrent DROP of
dropped_member.

Note that DropOwnedObjects() and ReassignOwnedObjects() check has_privs_of_role()
after the lock is acquired. Moving those into a callback could be done if we feel
the need.

[1]: https://postgr.es/m/CAM6Zo8woa62ZFHtMKox6a4jb8qQ%3Dw87R2L0K8347iE-juQL2EA%40mail.gmail.com

Regards,

---

From: surya poondla <[email-masked]>
Date: Wed, 8 Jul 2026 21:45:34 -0700

Hi Bertrand,

Thanks for the patch set. I agree the races are real, and reusing the
RangeVarGetRelidExtended() invalidation-retry idiom is a good fit, the
retry loop looks correct and closes the window for the covered paths.

I have a few comments
1. The 0002 patch creates a new deadlock. GrantRole() now locks the member
via roleSpecsToIds() (AccessShareLock, before the granted-role loop) and
then the granted role via RoleNameGetOid()
(ShareUpdateExclusiveLock).
DROP ROLE takes AccessExclusiveLock per role in list order.
Because AccessExclusiveLock conflicts with everything, this creates a
lock-order cycle that did not exist before.
For example say we have 2 concurrent sessions:
S1: GRANT g TO m;       S2: DROP ROLE g, m;
1. S1 acquires AccessShareLock(m)
2. S2 acquires AccessExclusiveLock(g)
3. S1 waits ShareUpdateExclusiveLock(g)  -- blocked by S2
4. S2 waits AccessExclusiveLock(m)           -- blocked by S1
-> deadlock
Pre-patch, GRANT didn't lock the member, so S2 never blocked S1.
The same expanded locking applies to CREATE ROLE ... ROLE and the ALTER
ROLE ... ADD/DROP USER (ALTER GROUP) path,
which also goes through roleSpecsToIds().
Swapping a silent orphan for a detected deadlock is arguably fine, but it's
a behavior change, could you document the lock ordering, and maybe have
DROP ROLE lock in a canonical (OID-sorted) order?

2. DROP ROLE now blocks on unrelated long transactions. The
AccessShareLock from
roleSpecsToIds() is held to commit,
so an open txn that ran GRANT/CREATE ROLE ... ROLE/REASSIGN OWNED touching
X, blocks a concurrent DROP ROLE X.  This is intended behavior, but worth a
note in the commit message/docs.

3. For non-cstring role specs, the else-branch (CURRENT_USER/SESSION_USER)
 does:
roleid = get_rolespec_oid(rolespec, false);
LockSharedObject(AuthIdRelationId, roleid, 0, AccessShareLock);
get_rolespec_oid() returns the backend's cached session OID (GetUserId())
rather than re-resolving a name, so there is no retry mechanism and
no post-lock existence check.
Since DropRole() only blocks dropping the *dropping* session's own user,
nothing stops another session from dropping this session's login role, so
"GRANT g TO CURRENT_USER" can still orphan.
RoleNameCallbackForDropRole() already does this by doing a re-check (a
SearchSysCache1(AUTHOID) after resolving, erroring if the tuple is gone),
so the else-branch could do the same after locking.

4. In role-membership-drop-member.spec only checks that the concurrent DROP
waits; it never asserts the outcome, so it would still pass if the locking
left an orphan. A final step selecting for orphaned rows would make it
detect outcome regressions, e.g.

       SELECT m.roleid, m.member, m.grantor
        FROM pg_auth_members m
        LEFT JOIN pg_authid ra ON m.roleid  = ra.oid
        LEFT JOIN pg_authid me ON m.member  = me.oid
        LEFT JOIN pg_authid gr ON m.grantor = gr.oid
       WHERE ra.oid IS NULL OR me.oid IS NULL OR gr.oid IS NULL;

Nit: the spec header comment has a stray '#' ("orphaned # pg_auth_members").

Regards,
Surya Poondla

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 9 Jul 2026 07:05:54 +0000

Hi Surya,

> retry loop looks correct and closes the window for the covered paths.

Thanks for looking at it!

> a behavior change, could you document the lock ordering

Nice catch! I think that a rare deadlock is better than a rare orphaned entry,
so I added a note in the commit message.

>, and maybe have

That would add extra complexity and I'm not sure that sorting only in DROP ROLE
would fully solve it.

Also, I don't think there is precedent in the code tree. So I think we should keep
it simple and just mention it in the commit message.

> note in the commit message/docs.

Right, added in the commit message. Not sure it's worth an addition in the doc
given that existing locking behavior for role commands is not documented there
either.

> so the else-branch could do the same after locking.

Good point, done in the attached.

> left an orphan.

I'm not sure how an orphan could be created if we ensure proper locking. That
said this extra check does not hurt, so added for the permutations that would
produce orphans without the patch.

Regards,