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

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