public inbox for [email protected]  

help / color / mirror / Atom feed

From: surya poondla <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: [email protected]
Subject: Re: Fix races conditions in DropRole() and GrantRole()
Date: Wed, 8 Jul 2026 21:45:34 -0700
Message-ID: <CAOVWO5prVyU4-LtN2VF=hqMVrg4BmXSG=S4khhwpp-AwRFqQ-w@mail.gmail.com> (raw)
In-Reply-To: <aki6fMNLUx6+BR8K@bdtpg>
References: <aki6fMNLUx6+BR8K@bdtpg>

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

view thread (4+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Fix races conditions in DropRole() and GrantRole()
  In-Reply-To: <CAOVWO5prVyU4-LtN2VF=hqMVrg4BmXSG=S4khhwpp-AwRFqQ-w@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox