Date: Thu, 9 Jul 2026 07:05:54 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: surya poondla <suryapoondla4@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Fix races conditions in DropRole() and GrantRole()
Message-ID: <ak9IUpaxNlhAflKU@bdtpg>
References: <aki6fMNLUx6+BR8K@bdtpg>
 <CAOVWO5prVyU4-LtN2VF=hqMVrg4BmXSG=S4khhwpp-AwRFqQ-w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="Dvg/WemKV0I3T/Od"
Content-Disposition: inline
In-Reply-To: 
 <CAOVWO5prVyU4-LtN2VF=hqMVrg4BmXSG=S4khhwpp-AwRFqQ-w@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ak9IUpaxNlhAflKU%40bdtpg>
Precedence: bulk


--Dvg/WemKV0I3T/Od
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi Surya,

On Wed, Jul 08, 2026 at 09:45:34PM -0700, surya poondla wrote:
> 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.

Thanks for looking at it!

> Swapping a silent orphan for a detected deadlock is arguably fine, but it's
> 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
> DROP ROLE lock in a canonical (OID-sorted) order?

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.

> 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.

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.

> 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.

Good point, done in the attached.

> 
> 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.

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,

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

--Dvg/WemKV0I3T/Od
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-Add-RoleNameGetOid-with-invalidation-based-retry-.patch"