Обсуждение: The presence of a NULL "defaclacl" value in pg_default_acl prevents the dropping of a role.
Hello postgres hackers:
I recently came across a scenario involving system catalog "pg_default_acl"
where a tuple contains a NULL value for the "defaclacl" attribute. This can cause
confusion while dropping a role whose default ACL has been changed.
Here is a way to reproduce that:
``` example
postgres=# create user adminuser;
CREATE ROLE
postgres=# create user normaluser;
CREATE ROLE
postgres=# alter default privileges for role adminuser grant all on tables to normaluser;
ALTER DEFAULT PRIVILEGES
postgres=# alter default privileges for role adminuser revoke all ON tables from adminuser;
ALTER DEFAULT PRIVILEGES
postgres=# alter default privileges for role adminuser revoke all ON tables from normaluser;
ALTER DEFAULT PRIVILEGES
postgres=# select * from pg_default_acl where pg_get_userbyid(defaclrole) = 'adminuser';
oid | defaclrole | defaclnamespace | defaclobjtype | defaclacl
-------+------------+-----------------+---------------+-----------
16396 | 16394 | 0 | r | {}
(1 row)
postgres=# drop user adminuser ;
ERROR: role "adminuser" cannot be dropped because some objects depend on it
DETAIL: owner of default privileges on new relations belonging to role adminuser
```
I believe this is a bug since the tuple can be deleted if we revoke from "normaluser"
first. Besides, according to the document:
https://www.postgresql.org/docs/current/sql-alterdefaultprivileges.html
> If you wish to drop a role for which the default privileges have been altered,
> it is necessary to reverse the changes in its default privileges or use DROP OWNED BY
> to get rid of the default privileges entry for the role.
There must be a way to "reverse the changes", but NULL value of "defaclacl"
prevents it. Luckily, "DROP OWNED BY" works well.
The code-level reason could be that the function "SetDefaultACL" doesn't handle
the situation where "new_acl" is NULL. So I present a simple patch here.
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 01ff575b093..0e313526b28 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -1335,7 +1335,7 @@ SetDefaultACL(InternalDefaultACL *iacls)
*/
aclitemsort(new_acl);
aclitemsort(def_acl);
- if (aclequal(new_acl, def_acl))
+ if (aclequal(new_acl, def_acl) || ACL_NUM(new_acl) == 0)
{
/* delete old entry, if indeed there is one */
if (!isNew)
Best regards,
Boyu Yang
"=?UTF-8?B?5p2o5Lyv5a6HKOmVv+Wggik=?=" <yangboyu.yby@alibaba-inc.com> writes: > postgres=# create user adminuser; > CREATE ROLE > postgres=# create user normaluser; > CREATE ROLE > postgres=# alter default privileges for role adminuser grant all on tables to normaluser; > ALTER DEFAULT PRIVILEGES > postgres=# alter default privileges for role adminuser revoke all ON tables from adminuser; > ALTER DEFAULT PRIVILEGES > postgres=# alter default privileges for role adminuser revoke all ON tables from normaluser; > ALTER DEFAULT PRIVILEGES > postgres=# select * from pg_default_acl where pg_get_userbyid(defaclrole) = 'adminuser'; > oid | defaclrole | defaclnamespace | defaclobjtype | defaclacl > -------+------------+-----------------+---------------+----------- > 16396 | 16394 | 0 | r | {} > (1 row) > postgres=# drop user adminuser ; > ERROR: role "adminuser" cannot be dropped because some objects depend on it > DETAIL: owner of default privileges on new relations belonging to role adminuser This looks perfectly normal to me: the privileges for 'adminuser' itself are not at the default state. If you then do regression=# alter default privileges for role adminuser grant all on tables to adminuser ; ALTER DEFAULT PRIVILEGES then things are back to normal, and the pg_default_acl entry goes away: regression=# select * from pg_default_acl; oid | defaclrole | defaclnamespace | defaclobjtype | defaclacl -----+------------+-----------------+---------------+----------- (0 rows) and you can drop the user: regression=# drop user adminuser ; DROP ROLE You could argue that there's no need to be picky about an entry that only controls privileges for the user-to-be-dropped, but it is working as designed and documented. I fear your proposed patch is likely to break more things than it fixes. In particular it looks like it would forget the existence of the user's self-revocation altogether, even before the drop of the user. regards, tom lane