From 4dbdb82513b65ab3a29e431f0233009c8a0fa890 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Thu, 22 Jun 2023 15:48:20 -0700 Subject: [PATCH] Fix cache lookup hazards introduced by ff9618e82a. ff9618e82a introduced has_partition_ancestor_privs(), which is used to check whether a user has MAINTAIN on any partition ancestors. This involves syscache lookups, and presently this function does not take any relation locks, so it is likely subject to the same kind of cache lookup failures that were fixed by 19de0ab23c. To fix this problem, this commit partially reverts ff9618e82a. Specifically, it removes the partition-related changes, including the has_partition_ancestor_privs() function mentioned above. This means that MAINTAIN on a partitioned table is no longer sufficient to perform maintenance commands on its partitions. This is more like how privileges for maintenance commands work on supported versions. Privileges are checked for each partition, so a command that flows down to all partitions might refuse to process them (e.g., if the current user doesn't have MAINTAIN on the partition). In passing, adjust a few related comments and error messages, and add a test for the privilege checks for CLUSTER on a partitioned table. Reviewed-by: Michael Paquier, Jeff Davis Discussion: https://postgr.es/m/20230613211246.GA219055%40nathanxps13 --- doc/src/sgml/ref/analyze.sgml | 5 +-- doc/src/sgml/ref/cluster.sgml | 5 +-- 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 | 12 ++++--- src/backend/commands/indexcmds.c | 27 +++++++-------- src/backend/commands/lockcmds.c | 8 ----- src/backend/commands/tablecmds.c | 34 +++---------------- src/backend/commands/vacuum.c | 10 ++---- src/include/commands/tablecmds.h | 1 - .../expected/cluster-conflict-partition.out | 14 ++++---- .../specs/cluster-conflict-partition.spec | 7 ++-- src/test/regress/expected/cluster.out | 7 +++- src/test/regress/expected/create_index.out | 4 +-- src/test/regress/expected/privileges.out | 8 ++--- src/test/regress/expected/vacuum.out | 18 ++++++++++ src/test/regress/sql/cluster.sql | 3 ++ 18 files changed, 74 insertions(+), 105 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 20c6f9939f..30a893230e 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -190,10 +190,7 @@ ANALYZE [ VERBOSE ] [ table_and_columnsANALYZE can only be performed by superusers and roles - 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. + with privileges of pg_maintain.) 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 29f0f1fd90..f0dd7faed5 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -137,10 +137,7 @@ CLUSTER [VERBOSE] 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 + role. CLUSTER will skip over any tables that the calling user does not have permission to cluster. diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 5b3b2b793a..8524182211 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -177,10 +177,7 @@ 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. 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. + MODE is permitted. diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 71455dfdc7..23f8c7630b 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -306,11 +306,7 @@ 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. 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. + privilege on the catalog. Of course, superusers can always reindex anything. diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 57bc4c23ec..445325e14c 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -452,10 +452,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ relname))); /* Check permissions */ - 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; + aclresult = pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_TABLE, relation->relname); } /* diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index bb79de4da6..7fe6a54c06 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,7 +41,6 @@ #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" @@ -721,17 +720,12 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, /*---------- * A role has privileges to vacuum or analyze the relation if any of the * following are true: - * - the role is a superuser - * - 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 + * - the role has the MAINTAIN privilege on the relation *---------- */ 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)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) return true; relname = NameStr(reltuple->relname); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 17b9404937..250d89ff88 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -99,7 +99,6 @@ 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 8d21276996..7be9e56ef1 100644 --- a/src/test/isolation/expected/cluster-conflict-partition.out +++ b/src/test/isolation/expected/cluster-conflict-partition.out @@ -3,7 +3,7 @@ Parsed test spec with 2 sessions starting permutation: s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; step s2_cluster: <... completed> @@ -11,7 +11,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; @@ -21,17 +21,15 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset 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_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; +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 s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; 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 ae38cb4ee3..4d38a7f49a 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -23,12 +23,15 @@ step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit { COMMIT; } session s2 -step s2_auth { SET ROLE regress_cluster_part; } +step s2_auth { SET ROLE regress_cluster_part; SET client_min_messages = ERROR; } step s2_cluster { CLUSTER cluster_part_tab USING cluster_part_ind; } step s2_reset { RESET ROLE; } -# CLUSTER waits if locked, passes for all cases. +# CLUSTER on the parent 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 2eec483eaa..a13aafff0b 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -502,12 +502,17 @@ CREATE TABLE ptnowner1 PARTITION OF ptnowner FOR VALUES IN (1); CREATE ROLE regress_ptnowner; CREATE TABLE ptnowner2 PARTITION OF ptnowner FOR VALUES IN (2); ALTER TABLE ptnowner1 OWNER TO regress_ptnowner; +SET SESSION AUTHORIZATION regress_ptnowner; +CLUSTER ptnowner USING ptnowner_i_idx; +ERROR: permission denied for table ptnowner +RESET SESSION AUTHORIZATION; ALTER TABLE ptnowner OWNER TO regress_ptnowner; CREATE TEMP TABLE ptnowner_oldnodes AS SELECT oid, relname, relfilenode FROM pg_partition_tree('ptnowner') AS tree JOIN pg_class AS c ON c.oid=tree.relid; SET SESSION AUTHORIZATION regress_ptnowner; CLUSTER ptnowner USING ptnowner_i_idx; +WARNING: permission denied to cluster "ptnowner2", skipping it RESET SESSION AUTHORIZATION; SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C"; @@ -515,7 +520,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | f + ptnowner2 | t (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index acfd9d1f4f..1473bc3175 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2831,9 +2831,9 @@ RESET ROLE; GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser; SET SESSION ROLE regress_reindexuser; REINDEX TABLE pg_toast.pg_toast_1260; -ERROR: must be owner of table pg_toast_1260 +ERROR: permission denied for table pg_toast_1260 REINDEX INDEX pg_toast.pg_toast_1260_index; -ERROR: must be owner of index pg_toast_1260_index +ERROR: permission denied for index pg_toast_1260_index -- Clean up RESET ROLE; REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 3cf4ac8c9e..3e4dfcc2ec 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -2928,13 +2928,13 @@ WARNING: permission denied to analyze "maintain_test", skipping it VACUUM (ANALYZE) maintain_test; WARNING: permission denied to vacuum "maintain_test", skipping it CLUSTER maintain_test USING maintain_test_a_idx; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REFRESH MATERIALIZED VIEW refresh_test; -ERROR: must be owner of table refresh_test +ERROR: permission denied for table refresh_test REINDEX TABLE maintain_test; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REINDEX INDEX maintain_test_a_idx; -ERROR: must be owner of index maintain_test_a_idx +ERROR: permission denied for index maintain_test_a_idx REINDEX SCHEMA reindex_test; ERROR: must be owner of schema reindex_test RESET ROLE; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 41e020cf20..4def90b805 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -442,14 +442,20 @@ 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; @@ -478,14 +484,26 @@ 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 a4cfaae807..b7115f8610 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -238,6 +238,9 @@ CREATE TABLE ptnowner1 PARTITION OF ptnowner FOR VALUES IN (1); CREATE ROLE regress_ptnowner; CREATE TABLE ptnowner2 PARTITION OF ptnowner FOR VALUES IN (2); ALTER TABLE ptnowner1 OWNER TO regress_ptnowner; +SET SESSION AUTHORIZATION regress_ptnowner; +CLUSTER ptnowner USING ptnowner_i_idx; +RESET SESSION AUTHORIZATION; ALTER TABLE ptnowner OWNER TO regress_ptnowner; CREATE TEMP TABLE ptnowner_oldnodes AS SELECT oid, relname, relfilenode FROM pg_partition_tree('ptnowner') AS tree