From ff9618e82a466fc9c635f9f087776e57b21e4f14 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Fri, 13 Jan 2023 15:32:37 -0800 Subject: [PATCH] Fix MAINTAIN privileges for toast tables and partitions. Commit 60684dd8 left loose ends when it came to maintaining toast tables or partitions. For toast tables, simply skip the privilege check if the toast table is an indirect target of the maintenance command, because the main table privileges have already been checked. For partitions, allow the maintenance command if the user has the MAINTAIN privilege on the partition or any parent. Also make CLUSTER emit "skipping" messages when the user doesn't have privileges, similar to VACUUM. Author: Nathan Bossart Reported-by: Pavel Luzanov Reviewed-by: Pavel Luzanov, Ted Yu Discussion: https://postgr.es/m/20230113231339.GA2422750@nathanxps13 --- doc/src/sgml/ref/analyze.sgml | 5 ++- doc/src/sgml/ref/cluster.sgml | 17 ++++++--- doc/src/sgml/ref/lock.sgml | 5 ++- doc/src/sgml/ref/reindex.sgml | 6 +++- doc/src/sgml/ref/vacuum.sgml | 5 ++- src/backend/commands/cluster.c | 35 +++++++++++++------ src/backend/commands/indexcmds.c | 22 ++++++------ src/backend/commands/lockcmds.c | 8 +++++ src/backend/commands/tablecmds.c | 30 ++++++++++++++-- src/backend/commands/vacuum.c | 21 ++++++----- src/include/commands/tablecmds.h | 1 + .../expected/cluster-conflict-partition.out | 6 ++-- .../specs/cluster-conflict-partition.spec | 5 +-- src/test/regress/expected/cluster.out | 4 ++- src/test/regress/expected/vacuum.out | 18 ---------- src/test/regress/sql/cluster.sql | 2 ++ 16 files changed, 126 insertions(+), 64 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index a26834da4f..2f94e89cb0 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -156,7 +156,10 @@ ANALYZE [ VERBOSE ] [ table_and_columnsANALYZE can only be performed by superusers and roles - with privileges of pg_maintain.) + with privileges of pg_maintain.) If a role has + permission to ANALYZE a partitioned table, it is also + permitted to ANALYZE each of its partitions, regardless + of whether the role has the aforementioned privileges on the partition. ANALYZE will skip over any tables that the calling user does not have permission to analyze. diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 145101e6a5..b9f2acb1de 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -69,10 +69,7 @@ CLUSTER [VERBOSE] CLUSTER without any parameter reclusters all the previously-clustered tables in the current database that the calling user - owns or has the MAINTAIN privilege for, or all such tables - if called by a superuser or a role with privileges of the - pg_maintain - role. This form of CLUSTER cannot be + has privileges for. This form of CLUSTER cannot be executed inside a transaction block. @@ -134,6 +131,18 @@ CLUSTER [VERBOSE] Notes + + To cluster a table, one must have the MAINTAIN privilege + on the table or be the table's owner, a superuser, or a role with + privileges of the + pg_maintain + role. If a role has permission to CLUSTER a partitioned + table, it is also permitted to CLUSTER each of its + partitions, regardless of whether the role has the aforementioned + privileges on the partition. CLUSTER will skip over any + tables that the calling user does not have permission to cluster. + + In cases where you are accessing single rows randomly within a table, the actual order of the data in the diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 8524182211..5b3b2b793a 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -177,7 +177,10 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] MODE (or a less-conflicting mode as described in ) is permitted. If a user has SELECT privileges on the table, ACCESS SHARE - MODE is permitted. + MODE is permitted. If a role has permission to lock a + partitioned table, it is also permitted to lock each of its partitions, + regardless of whether the role has the aforementioned privileges on the + partition. diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 192513f34e..c6ad2546f9 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -306,7 +306,11 @@ REINDEX [ ( option [, ...] ) ] { DA indexes on shared catalogs will be skipped unless the user owns the catalog (which typically won't be the case), has privileges of the pg_maintain role, or has the MAINTAIN - privilege on the catalog. Of course, superusers can always reindex anything. + privilege on the catalog. If a role has permission to + REINDEX a partitioned table, it is also permitted to + REINDEX each of its partitions, regardless of whether the + role has the aforementioned privileges on the partition. Of course, + superusers can always reindex anything. diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 8fa8421847..545b23b54f 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -401,7 +401,10 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ relname); @@ -3008,16 +3008,16 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, /* * The table can be reindexed if the user has been granted MAINTAIN on - * the table or the user is a superuser, the table owner, or the - * database/schema owner (but in the latter case, only if it's not a - * shared relation). object_ownercheck includes the superuser case, - * and depending on objectKind we already know that the user has - * permission to run REINDEX on this database or schema per the - * permission checks at the beginning of this routine. + * the table or one of its partition ancestors or the user is a + * superuser, the table owner, or the database/schema owner (but in the + * latter case, only if it's not a shared relation). pg_class_aclcheck + * includes the superuser case, and depending on objectKind we already + * know that the user has permission to run REINDEX on this database or + * schema per the permission checks at the beginning of this routine. */ if (classtuple->relisshared && - !object_ownercheck(RelationRelationId, relid, GetUserId()) && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && + !has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) continue; /* diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 410d78b040..6bf1b815f0 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,6 +19,7 @@ #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" +#include "commands/tablecmds.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "parser/parse_clause.h" @@ -305,5 +306,12 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid) aclresult = pg_class_aclcheck(reloid, userid, aclmask); + /* + * If this is a partition, check permissions of its ancestors if needed. + */ + if (aclresult != ACLCHECK_OK && + has_partition_ancestor_privs(reloid, userid, ACL_MAINTAIN)) + aclresult = ACLCHECK_OK; + return aclresult; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 1fbdad4b64..7c697a285b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -16886,12 +16886,38 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, errmsg("\"%s\" is not a table or materialized view", relation->relname))); /* Check permissions */ - if (!object_ownercheck(RelationRelationId, relId, GetUserId()) && - pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) + if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && + !has_partition_ancestor_privs(relId, GetUserId(), ACL_MAINTAIN)) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, relation->relname); } +/* + * If relid is a partition, returns whether userid has any of the privileges + * specified in acl on any of its ancestors. Otherwise, returns false. + */ +bool +has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl) +{ + List *ancestors; + ListCell *lc; + + if (!get_rel_relispartition(relid)) + return false; + + ancestors = get_partition_ancestors(relid); + foreach(lc, ancestors) + { + Oid ancestor = lfirst_oid(lc); + + if (OidIsValid(ancestor) && + pg_class_aclcheck(ancestor, userid, acl) == ACLCHECK_OK) + return true; + } + + return false; +} + /* * Callback to RangeVarGetRelidExtended() for TRUNCATE processing. */ diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index ea1428dc8c..7b1a4b127e 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,6 +41,7 @@ #include "catalog/pg_namespace.h" #include "commands/cluster.h" #include "commands/defrem.h" +#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -91,7 +92,8 @@ static void vac_truncate_clog(TransactionId frozenXID, MultiXactId minMulti, TransactionId lastSaneFrozenXid, MultiXactId lastSaneMinMulti); -static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params); +static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, + bool skip_privs); static double compute_parallel_delay(void); static VacOptValue get_vacoptval_from_boolean(DefElem *def); static bool vac_tid_reaped(ItemPointer itemptr, void *state); @@ -501,7 +503,7 @@ vacuum(List *relations, VacuumParams *params, if (params->options & VACOPT_VACUUM) { - if (!vacuum_rel(vrel->oid, vrel->relation, params)) + if (!vacuum_rel(vrel->oid, vrel->relation, params, false)) continue; } @@ -598,11 +600,13 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, * - the role owns the relation * - the role owns the current database and the relation is not shared * - the role has been granted the MAINTAIN privilege on the relation + * - the role has privileges to vacuum/analyze any of the relation's + * partition ancestors *---------- */ - if (object_ownercheck(RelationRelationId, relid, GetUserId()) || - (object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) + if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK || + has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) return true; relname = NameStr(reltuple->relname); @@ -1828,7 +1832,7 @@ vac_truncate_clog(TransactionId frozenXID, * At entry and exit, we are not inside a transaction. */ static bool -vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) +vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs) { LOCKMODE lmode; Relation rel; @@ -1915,7 +1919,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) * happen across multiple transactions where privileges could have changed * in-between. Make sure to only generate logs for VACUUM in this case. */ - if (!vacuum_is_permitted_for_relation(RelationGetRelid(rel), + if (!skip_privs && + !vacuum_is_permitted_for_relation(RelationGetRelid(rel), rel->rd_rel, params->options & VACOPT_VACUUM)) { @@ -2089,7 +2094,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) * totally unimportant for toast relations. */ if (toast_relid != InvalidOid) - vacuum_rel(toast_relid, NULL, params); + vacuum_rel(toast_relid, NULL, params, true); /* * Now release the session-level lock on the main table. diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 2e717fa815..e7c2b91a58 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -98,6 +98,7 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit, extern void RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); +extern bool has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl); extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out index 7acb675c97..8d21276996 100644 --- a/src/test/isolation/expected/cluster-conflict-partition.out +++ b/src/test/isolation/expected/cluster-conflict-partition.out @@ -22,14 +22,16 @@ starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_res step s1_begin: BEGIN; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s2_auth: SET ROLE regress_cluster_part; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; +step s2_cluster: <... completed> step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s2_auth: SET ROLE regress_cluster_part; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; +step s2_cluster: <... completed> step s2_reset: RESET ROLE; diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec index 5091f684a9..ae38cb4ee3 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -27,11 +27,8 @@ step s2_auth { SET ROLE regress_cluster_part; } step s2_cluster { CLUSTER cluster_part_tab USING cluster_part_ind; } step s2_reset { RESET ROLE; } -# CLUSTER on the parent waits if locked, passes for all cases. +# CLUSTER waits if locked, passes for all cases. permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset - -# When taking a lock on a partition leaf, CLUSTER on the parent skips -# the leaf, passes for all cases. permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 542c2e098c..2eec483eaa 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -352,7 +352,9 @@ INSERT INTO clstr_3 VALUES (1); -- this user can only cluster clstr_1 and clstr_3, but the latter -- has not been clustered SET SESSION AUTHORIZATION regress_clstr_user; +SET client_min_messages = ERROR; -- order of "skipping" warnings may vary CLUSTER; +RESET client_min_messages; SELECT * FROM clstr_1 UNION ALL SELECT * FROM clstr_2 UNION ALL SELECT * FROM clstr_3; @@ -513,7 +515,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | t + ptnowner2 | f (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index d860be0e20..458adee7f8 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -353,20 +353,14 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; -WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; -WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; -WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; -WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; -WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; -WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; @@ -395,26 +389,14 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; -WARNING: permission denied to vacuum "vacowned_part1", skipping it -WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; -WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; -WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; -WARNING: permission denied to analyze "vacowned_part1", skipping it -WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; -WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; -WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; -WARNING: permission denied to vacuum "vacowned_part1", skipping it -WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; -WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; -WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql index 6cb9c926c0..a4cfaae807 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -145,7 +145,9 @@ INSERT INTO clstr_3 VALUES (1); -- this user can only cluster clstr_1 and clstr_3, but the latter -- has not been clustered SET SESSION AUTHORIZATION regress_clstr_user; +SET client_min_messages = ERROR; -- order of "skipping" warnings may vary CLUSTER; +RESET client_min_messages; SELECT * FROM clstr_1 UNION ALL SELECT * FROM clstr_2 UNION ALL SELECT * FROM clstr_3;