On 27/01/2026 10:10 PM, Konstantin Knizhnik wrote:
On 22/01/2026 6:35 PM, Tom Lane wrote:
Konstantin Knizhnik <[email protected]> writes:
But I wonder if we do refactoring of this revoke privileges stuff,
should we also provide correct (expected) behaviour in case of missing
grantor specification. i.e.
revoke all privileges on table <T> from <role>;
If privileges to access this table were granted to this role by
multiple
grantors, then it is natural to expect that the statement above will
remove all such grants and so as a result <role> can not access this
table any more, rather than try to find best grantor and finally still
leave privileges for this role, isn't it?
Unfortunately, the SQL spec is quite clear that REVOKE revokes only
privileges granted directly by the calling user (or the GRANTED BY
role, if that's given). We're already far outside the spec by
allowing select_best_grantor to locate an inherited role to do the
revoke as. I can't see reinterpreting it as "revoke all privileges
granted by anybody", even assuming that the calling user has
sufficient permissions to do that.
regards, tom lane
Can I ask one more question.
What do you think about the following (similar) scenario:
create role creator superuser;
set role creator;
create role reader;
create role somebody;
grant reader to somebody;
grant creator to somebody;
create table t(x integer);
grant select on table t to somebody with grant option;
begin;
set local role somebody;
grant select on table t to reader;
commit;
drop owned by reader cascade;
drop role reader;
ERROR: role "reader" cannot be dropped because some objects depend on it
DETAIL: privileges for table t
What standard is saying about DROP OWNER BY ... CASCADE?
Should it delete reader's privileges in this case?
There is simple "know-how" in Postgres how to drop role having
dependent objects:
REASSIGN OWNED BY ... TO ...;
DROP OWNED BY ...;
But it doesn't work in the case above.
It it necessary to manually locate and drop all granted privileges.
And there are more than 10 kind of objects in Postgres to which
privileges is granted.
So if you need to write procedure which is guaranteed to drop any
role, then there is no simple solution, is it?
I can propose such straightforward patch fixing this issue with "DROP
OWNED BY...":
it just removes from ACL any items with specified grantee.
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 24948c1f05e..9dba10e5dc8 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -179,7 +179,7 @@ static void recordExtensionInitPrivWorker(Oid objoid, Oid
classoid, int objsubid
* NB: the original old_acl is pfree'd.
*/
static Acl *
-merge_acl_with_grant(Acl *old_acl, bool is_grant,
+merge_acl_with_grant(Acl *old_acl, bool is_grant, bool is_remove_role,
bool grant_option, DropBehavior
behavior,
List *grantees, AclMode privileges,
Oid grantorId, Oid ownerId)
@@ -223,7 +223,7 @@ merge_acl_with_grant(Acl *old_acl, bool is_grant,
(is_grant ||
!grant_option) ? privileges : ACL_NO_RIGHTS,
(!is_grant
|| grant_option) ? privileges : ACL_NO_RIGHTS);
- newer_acl = aclupdate(new_acl, &aclitem, modechg, ownerId,
behavior);
+ newer_acl = aclupdate(new_acl, &aclitem, modechg, ownerId,
behavior, is_remove_role);
/* avoid memory leak when there are many grantees */
pfree(new_acl);
@@ -416,6 +416,7 @@ ExecuteGrantStmt(GrantStmt *stmt)
* Turn the regular GrantStmt into the InternalGrant form.
*/
istmt.is_grant = stmt->is_grant;
+ istmt.is_remove_role = false;
istmt.objtype = stmt->objtype;
/* Collect the OIDs of the target objects */
@@ -1281,6 +1282,7 @@ SetDefaultACL(InternalDefaultACL *iacls)
*/
new_acl = merge_acl_with_grant(old_acl,
iacls->is_grant,
+ false,
iacls->grant_option,
iacls->behavior,
iacls->grantees,
@@ -1535,6 +1537,7 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid
objid)
break;
}
istmt.is_grant = false;
+ istmt.is_remove_role = true;
istmt.objects = list_make1_oid(objid);
istmt.all_privs = true;
istmt.privileges = ACL_NO_RIGHTS;
@@ -1720,7 +1723,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid,
const char *relname,
/*
* Generate new ACL.
*/
- new_acl = merge_acl_with_grant(old_acl, istmt->is_grant,
+ new_acl = merge_acl_with_grant(old_acl, istmt->is_grant, false,
istmt->grant_option,
istmt->behavior, istmt->grantees,
col_privileges, grantorId,
@@ -1998,6 +2001,7 @@ ExecGrant_Relation(InternalGrant *istmt)
*/
new_acl = merge_acl_with_grant(old_acl,
istmt->is_grant,
+
istmt->is_remove_role,
istmt->grant_option,
istmt->behavior,
istmt->grantees,
@@ -2204,7 +2208,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid,
AclMode default_privs,
/*
* Generate new ACL.
*/
- new_acl = merge_acl_with_grant(old_acl, istmt->is_grant,
+ new_acl = merge_acl_with_grant(old_acl, istmt->is_grant, false,
istmt->grant_option, istmt->behavior,
istmt->grantees, this_privileges,
grantorId, ownerId);
@@ -2356,7 +2360,7 @@ ExecGrant_Largeobject(InternalGrant *istmt)
/*
* Generate new ACL.
*/
- new_acl = merge_acl_with_grant(old_acl, istmt->is_grant,
+ new_acl = merge_acl_with_grant(old_acl, istmt->is_grant, false,
istmt->grant_option, istmt->behavior,
istmt->grantees, this_privileges,
grantorId, ownerId);
@@ -2503,7 +2507,7 @@ ExecGrant_Parameter(InternalGrant *istmt)
/*
* Generate new ACL.
*/
- new_acl = merge_acl_with_grant(old_acl, istmt->is_grant,
+ new_acl = merge_acl_with_grant(old_acl, istmt->is_grant, false,
istmt->grant_option, istmt->behavior,
istmt->grantees, this_privileges,
grantorId, ownerId);
@@ -4946,6 +4950,7 @@ RemoveRoleFromInitPriv(Oid roleid, Oid classid, Oid
objid, int32 objsubid)
if (old_acl != NULL)
new_acl = merge_acl_with_grant(old_acl,
false, /* is_grant */
+
false, /* is_remove_role */
false, /* grant_option */
DROP_RESTRICT,
list_make1_oid(roleid),
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index fc6f953d51a..ce38019e614 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -557,7 +557,7 @@ aclmerge(const Acl *left_acl, const Acl *right_acl, Oid
ownerId)
Acl *tmp_acl;
tmp_acl = aclupdate(result_acl, aip, ACL_MODECHG_ADD,
- ownerId, DROP_RESTRICT);
+ ownerId, DROP_RESTRICT,
false);
pfree(result_acl);
result_acl = tmp_acl;
}
@@ -1017,7 +1017,7 @@ acldefault_sql(PG_FUNCTION_ARGS)
*/
Acl *
aclupdate(const Acl *old_acl, const AclItem *mod_aip,
- int modechg, Oid ownerId, DropBehavior behavior)
+ int modechg, Oid ownerId, DropBehavior behavior, bool
is_remove_role)
{
Acl *new_acl = NULL;
AclItem *old_aip,
@@ -1040,6 +1040,23 @@ aclupdate(const Acl *old_acl, const AclItem *mod_aip,
num = ACL_NUM(old_acl);
old_aip = ACL_DAT(old_acl);
+ if (is_remove_role)
+ {
+ int src;
+ new_acl = allocacl(num);
+ new_aip = ACL_DAT(new_acl);
+ for (dst = src = 0; src < num; src += 1)
+ {
+ if (mod_aip->ai_grantee != old_aip[src].ai_grantee)
+ {
+ memmove(&new_aip[dst++], &old_aip[src],
sizeof(AclItem));
+ }
+ }
+ /* Adjust array size to be 'num - 1' items */
+ ARR_DIMS(new_acl)[0] = dst;
+ SET_VARSIZE(new_acl, ACL_N_SIZE(dst));
+ return new_acl;
+ }
/*
* Search the ACL for an existing entry for this grantee and grantor. If
* one exists, just modify the entry in-place (well, in the same
position,
@@ -1284,7 +1301,7 @@ cc_restart:
/* We'll actually zap ordinary privs too, but no matter
*/
new_acl = aclupdate(acl, &aip[i], ACL_MODECHG_DEL,
- ownerId,
DROP_CASCADE);
+ ownerId,
DROP_CASCADE, false);
pfree(acl);
acl = new_acl;
@@ -1375,7 +1392,7 @@ restart:
revoke_privs);
new_acl = aclupdate(acl, &mod_acl, ACL_MODECHG_DEL,
- ownerId,
behavior);
+ ownerId,
behavior, false);
pfree(acl);
acl = new_acl;
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 01ae5b719fd..89f9ed1f840 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -196,7 +196,7 @@ extern void recordDependencyOnNewAcl(Oid classId, Oid
objectId, int32 objsubId,
Oid
ownerId, Acl *acl);
extern Acl *aclupdate(const Acl *old_acl, const AclItem *mod_aip,
- int modechg, Oid ownerId,
DropBehavior behavior);
+ int modechg, Oid ownerId,
DropBehavior behavior, bool is_remove_role);
extern Acl *aclnewowner(const Acl *old_acl, Oid oldOwnerId, Oid newOwnerId);
extern Acl *make_empty_acl(void);
extern Acl *aclcopy(const Acl *orig_acl);
diff --git a/src/include/utils/aclchk_internal.h
b/src/include/utils/aclchk_internal.h
index 62af290779a..16c33c0a25b 100644
--- a/src/include/utils/aclchk_internal.h
+++ b/src/include/utils/aclchk_internal.h
@@ -31,6 +31,7 @@
typedef struct
{
bool is_grant;
+ bool is_remove_role;
ObjectType objtype;
List *objects;
bool all_privs;
