MIME-Version: 1.0
References: <aki6fMNLUx6+BR8K@bdtpg>
In-Reply-To: <aki6fMNLUx6+BR8K@bdtpg>
From: surya poondla <suryapoondla4@gmail.com>
Date: Wed, 8 Jul 2026 21:45:34 -0700
Message-ID: 
 <CAOVWO5prVyU4-LtN2VF=hqMVrg4BmXSG=S4khhwpp-AwRFqQ-w@mail.gmail.com>
Subject: Re: Fix races conditions in DropRole() and GrantRole()
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="00000000000024aec30656264cec"
Archived-At: 
 <https://www.postgresql.org/message-id/CAOVWO5prVyU4-LtN2VF%3DhqMVrg4BmXSG%3DS4khhwpp-AwRFqQ-w%40mail.gmail.com>
Precedence: bulk

--00000000000024aec30656264cec
Content-Type: text/plain; charset="UTF-8"

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

--00000000000024aec30656264cec
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi Bertrand,<div><br></div><div><br></div><div>Thanks for =
the patch set. I agree the races are real, and reusing the RangeVarGetRelid=
Extended() invalidation-retry idiom is a good fit, the retry loop looks cor=
rect and closes the window for the covered paths.<br><br>I have a few comme=
nts</div><div>1. The 0002 patch creates a new deadlock. GrantRole() now loc=
ks the member via roleSpecsToIds() (AccessShareLock, before the granted-rol=
e loop) and then the granted role via RoleNameGetOid()<br>(ShareUpdateExclu=
siveLock).</div><div>DROP ROLE takes AccessExclusiveLock per role in list o=
rder. Because=C2=A0AccessExclusiveLock conflicts with everything, this crea=
tes a lock-order cycle that did not exist before.=C2=A0</div><div>For examp=
le say we have 2 concurrent sessions:</div><div>S1: GRANT g TO m; =C2=A0 =
=C2=A0 =C2=A0 S2: DROP ROLE g, m;<br>1. S1 acquires AccessShareLock(m)=C2=
=A0<br>2. S2 acquires AccessExclusiveLock(g)=C2=A0 =C2=A0<br>3. S1 waits Sh=
areUpdateExclusiveLock(g) =C2=A0-- blocked by S2<br>4. S2 waits AccessExclu=
siveLock(m)=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0-- blocked by S1<span s=
tyle=3D"background-color:transparent"></span></div><div><span style=3D"back=
ground-color:transparent">-&gt; deadlock</span><br></div><div>Pre-patch, GR=
ANT didn&#39;t lock the member, so S2 never blocked S1.=C2=A0</div><div><sp=
an style=3D"background-color:transparent">The same=C2=A0</span><span style=
=3D"background-color:transparent">expanded locking applies to CREATE ROLE .=
.. ROLE and the ALTER ROLE ... ADD/DROP USER (ALTER GROUP) path,=C2=A0</spa=
n></div><div><span style=3D"background-color:transparent">which also </span=
>goes through<span style=3D"background-color:transparent"> roleSpecsToIds()=
.=C2=A0</span></div><div><span style=3D"background-color:transparent">Swapp=
ing a silent orphan for a detected deadlock is arguably fine, but it&#39;s =
a behavior change, could you document the lock ordering, and maybe have DRO=
P ROLE lock in a canonical (OID-sorted) order?</span></div><div><span style=
=3D"background-color:transparent"><br></span></div><div><span style=3D"back=
ground-color:transparent">2.=C2=A0</span><span style=3D"background-color:tr=
ansparent">DROP ROLE now blocks on unrelated long transactions. The AccessS=
hareLock=C2=A0</span><span style=3D"background-color:transparent">from role=
SpecsToIds() is held to commit,=C2=A0</span></div><div><span style=3D"backg=
round-color:transparent">so an open txn that ran </span><span style=3D"back=
ground-color:transparent">GRANT/CREATE ROLE ... ROLE/REASSIGN OWNED touchin=
g X, blocks a concurrent DROP ROLE X.=C2=A0=C2=A0</span><span style=3D"back=
ground-color:transparent">This is intended behavior, but worth a note in th=
e commit message/docs.</span></div><div><span style=3D"background-color:tra=
nsparent"><br></span></div><div><span style=3D"background-color:transparent=
">3. F</span><span style=3D"background-color:transparent">or non-cstring ro=
le specs, the else-branch (</span>CURRENT_USER/SESSION_USER)<span style=3D"=
background-color:transparent">=C2=A0does</span><span style=3D"background-co=
lor:transparent">:</span></div>roleid =3D get_rolespec_oid(rolespec, false)=
;<br>LockSharedObject(AuthIdRelationId, roleid, 0, AccessShareLock);<div>ge=
t_rolespec_oid() returns the backend&#39;s cached session OID (GetUserId())=
 rather than re-resolving a name, so there is no retry mechanism and no=C2=
=A0post-lock existence check.=C2=A0</div><div>Since DropRole() only blocks =
dropping the *dropping* session&#39;s own user, nothing stops another sessi=
on from dropping this session&#39;s login role, so &quot;GRANT g TO CURRENT=
_USER&quot; can still orphan.=C2=A0</div>RoleNameCallbackForDropRole() alre=
ady does this by doing a re-check (a SearchSysCache1(AUTHOID) after resolvi=
ng, erroring if the tuple=C2=A0<span style=3D"background-color:transparent"=
>is gone), so the else-branch could do the same after locking.</span><div><=
div><span style=3D"background-color:transparent"><br></span></div><div><spa=
n style=3D"background-color:transparent">4. In=C2=A0</span>role-membership-=
drop-member.spec only checks that the concurrent DROP waits; it never asser=
ts the outcome, so it would still pass if the locking left an orphan. A fin=
al step selecting for orphaned rows would make it detect outcome regression=
s, e.g.</div><div><br>=C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT m.roleid, m.member,=
 m.grantor<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 FROM pg_auth_members m<br>=C2=A0 =
=C2=A0 =C2=A0 =C2=A0 LEFT JOIN pg_authid ra ON m.roleid =C2=A0=3D ra.oid<br=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 LEFT JOIN pg_authid me ON m.member =C2=A0=3D m=
e.oid<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 LEFT JOIN pg_authid gr ON m.grantor =
=3D gr.oid<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0WHERE ra.oid IS NULL OR me.oid IS =
NULL OR gr.oid IS NULL;<span style=3D"background-color:transparent"><br><br=
></span>Nit: the spec header comment has a stray &#39;#&#39; (&quot;orphane=
d # pg_auth_members&quot;).<span style=3D"background-color:transparent"></s=
pan></div><div><span style=3D"background-color:transparent"><br></span></di=
v><div>Regards,<br>Surya Poondla</div><div></div></div></div>

--00000000000024aec30656264cec--