The first sentence is still "The grantee might still have some grant options via another grantor," and I don't think adding "direct" would have helped me understand this any better the first time I read it. I'm definitely up for more bikeshedding, though, because I don't really like what I have...
Fair. The part that tripped me up was actually the second sentence, "granted by any role on the chain", since the code isn't checking whether some role could grant them, but whether the grantee itself still holds them directly via another grantor. Maybe something like "if the grantee still holds a grant option directly through another grantor, that privilege's chain is intact"? No strong opinion though.
> On check_circularity() for [1]: I tried the same aclmask_direct() swap, > but since it runs on every GRANT ... WITH GRANT OPTION, which > pg_dump/restore replays, erroring there could make restore/pg_upgrade of > an existing cluster (one already holding the [1] self-grant) fail.
Well, I think the complaint in [1] is that dump/restore *already* fails, no? Can you provide an example of a correct (or benignly incorrect) dump that would start failing?
The already-failing case in [1] is the one where the membership has been revoked (REVOKE member FROM owner), which leaves a true orphan. But the self-grant gets created as soon as an inheriting member does the redundant WITH GRANT OPTION self-grant, and if the membership is still in place, that cluster dumps and restores fine today.
CREATE ROLE owner_role; CREATE ROLE member_role LOGIN; GRANT owner_role TO member_role; -- membership kept CREATE TABLE t (i int); ALTER TABLE t OWNER TO owner_role; GRANT SELECT ON t TO member_role WITH GRANT OPTION; SET ROLE member_role; GRANT SELECT ON t TO member_role WITH GRANT OPTION; -- self-grant
pg_dump emits "SET SESSION AUTHORIZATION member_role; GRANT SELECT ON t TO member_role WITH GRANT OPTION;". Restoring that into a fresh cluster works today, but with the check_circularity() change it fails with "grant options cannot be granted back to your own grantor".
So it's not that [1] is fine today; it's that the fix also breaks a broader set of clusters that currently round-trip (membership still present), including via pg_upgrade. That's why I think the check_circularity() side needs a companion dump/restore change rather than going in on its own.
As an aside, I'm not sure if check_circularity() is correctly preventing cycles independently of this issue, so that part may end up spiraling a bit.
