Hi Bertrand,
Thanks for the v3 patch. The commit-message notes on the deadlock, the DROP ROLE's blocking behavior, and the orphan-count checks in the isolation test, all
look good to me.
Couple of comments on the new recheck in roleSpecsToIds():
1. In the error path, the else-branch currently does:
if (!SearchSysCacheExists1(AUTHOID, ObjectIdGetDatum(roleid)))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("role \"%s\" does not exist",
get_rolespec_name(rolespec))));
The existence check itself is correct, but get_rolespec_name(rolespec) calls get_rolespec_tuple(), which for CURRENT_USER/SESSION_USER does
SearchSysCache1(AUTHOID, GetUserId()) and, if the tuple is missing, throws elog(ERROR, "cache lookup failed for role %u").
Since we only reach this branch because the role no longer exists, building the message re-looks-up the dropped role and fails with that internal error
instead of the intended "role ... does not exist" i.e. it emits exactly the kind of internal error this patch set is meant to remove.
Instead we can report the OID and it avoids the re-lookup entirely, e.g.
if (!SearchSysCacheExists1(AUTHOID, ObjectIdGetDatum(roleid)))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("role with OID %u does not exist", roleid)));
Capturing get_rolespec_name() before locking would only help the concurrent-drop case.
If the session's own role was already dropped earlier, get_rolespec_oid() still returns the stale cached OID and
get_rolespec_name() would fail the same way, so the OID form is the robust one.
2. The new s2_check_orphans permutations all use named roles, which go
through the RoleNameGetOid() (CSTRING) path. None exercise the CURRENT_USER/SESSION_USER else-branch that the recheck was added for.
A permutation like "GRANT g TO CURRENT_USER" with a concurrent drop of the session's login
role would cover that else branch and this would also exercise the path in point #1
Regards,
Surya Poondla
