Attached is a minimal patch to allow missing roles in REVOKE command This should fix the pg_upgrade issue and also a case where somebody has dropped a role you are trying to revoke privileges from :
smalltest=# create table revoketest(); CREATE TABLE smalltest=# revoke select on revoketest from bob; WARNING: ignoring REVOKE FROM a missing role "bob" REVOKE smalltest=# create user bob; CREATE ROLE smalltest=# grant select on revoketest to bob; GRANT smalltest=# \du List of roles Role name | Attributes -----------+------------------------------------------------------------ bob | hannuk | Superuser, Create role, Create DB, Replication, Bypass RLS smalltest=# \dp Access privileges Schema | Name | Type | Access privileges | Column privileges | Policies --------+------------+-------+------------------------+-------------------+---------- public | revoketest | table | hannuk=arwdDxtm/hannuk+| | | | | bob=r/hannuk | | public | vacwatch | table | | | (2 rows) smalltest=# revoke select on revoketest from bob, joe; WARNING: ignoring REVOKE FROM a missing role "joe" REVOKE smalltest=# \dp Access privileges Schema | Name | Type | Access privileges | Column privileges | Policies --------+------------+-------+------------------------+-------------------+---------- public | revoketest | table | hannuk=arwdDxtm/hannuk | | public | vacwatch | table | | | (2 rows) On Sun, May 26, 2024 at 12:05 AM Hannu Krosing <han...@google.com> wrote: > > On Sat, May 25, 2024 at 4:48 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > Hannu Krosing <han...@google.com> writes: > > > Having an pg_init_privs entry referencing a non-existing user is > > > certainly of no practical use. > > > > Sure, that's not up for debate. What I think we're discussing > > right now is > > > > 1. What other cases are badly handled by the pg_init_privs > > mechanisms. > > > > 2. How much of that is practical to fix in v17, seeing that > > it's all long-standing bugs and we're already past beta1. > > > > I kind of doubt that the answer to #2 is "all of it". > > But perhaps we can do better than "none of it". > > Putting the fix either in pg_dump or making REVOKE tolerate > non-existing users would definitely be most practical / useful fixes, > as these would actually allow pg_upgrade to v17 to work without > changing anything in older versions. > > Currently one already can revoke a privilege that is not there in the > first place, with the end state being that the privilege (still) does > not exist. > This does not even generate a warning. > > Extending this to revoking from users that do not exist does not seem > any different on conceptual level, though I understand that > implementation would be very different as it needs catching the user > lookup error from a very different part of the code. > > That said, it would be better if we can have something that would be > easy to backport something that would make pg_upgrade work for all > supported versions. > Making REVOKE silently ignore revoking from non-existing users would > improve general robustness but could conceivably change behaviour if > somebody relies on it in their workflows. > > Regards, > Hannu
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 7abf3c2a74..aff91582d7 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -444,6 +444,9 @@ ExecuteGrantStmt(GrantStmt *stmt) * Convert the RoleSpec list into an Oid list. Note that at this point we * insert an ACL_ID_PUBLIC into the list if appropriate, so downstream * there shouldn't be any additional work needed to support this case. + * + * Allow missing grantees in case of REVOKE (!istmt.is_grant) + * if now valid roles found return immediately */ foreach(cell, stmt->grantees) { @@ -456,12 +459,20 @@ ExecuteGrantStmt(GrantStmt *stmt) grantee_uid = ACL_ID_PUBLIC; break; default: - grantee_uid = get_rolespec_oid(grantee, false); + grantee_uid = get_rolespec_oid(grantee, !istmt.is_grant); break; } - istmt.grantees = lappend_oid(istmt.grantees, grantee_uid); + if (OidIsValid(grantee_uid)) + istmt.grantees = lappend_oid(istmt.grantees, grantee_uid); + else + ereport(WARNING, + (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION), + errmsg("ignoring REVOKE FROM a missing role \"%s\"", grantee->rolename))); } + if (istmt.grantees == NIL) + return; + /* * Convert stmt->privileges, a list of AccessPriv nodes, into an AclMode * bitmask. Note: objtype can't be OBJECT_COLUMN.