Here is a new version of the patch. Besides some cleanup, I added an index
on reltoastrelid for the toast-to-main-relation lookup. Before I bother
adjusting the tests and documentation, I'm curious to hear thoughts on
whether this seems like a viable approach.
On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> +cluster_is_permitted_for_relation(Oid relid, Oid userid)
> +{
> + return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
> ACLCHECK_OK ||
> + has_parent_privs(relid, userid, ACL_MAINTAIN);
>
> Since the func only contains one statement, it seems this can be defined as
> a macro instead.
In the new version, there is a bit more to this function, so I didn't
convert it to a macro.
> + List *ancestors = get_partition_ancestors(relid);
> + Oid root = InvalidOid;
>
> nit: it would be better if the variable `root` can be aligned with variable
> `ancestors`.
Hm. It looked alright on my machine. In any case, I'll be sure to run
pgindent at some point. This patch is still in early stages.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From f0f65618d1772151b3c4aff6f8da77bd1f212278 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Sun, 18 Dec 2022 14:46:21 -0800
Subject: [PATCH v2 1/1] fix maintain privs
---
src/backend/catalog/partition.c | 22 ++++++++++
src/backend/catalog/toasting.c | 32 +++++++++++++++
src/backend/commands/cluster.c | 35 +++++++++++-----
src/backend/commands/indexcmds.c | 10 ++---
src/backend/commands/lockcmds.c | 5 +++
src/backend/commands/tablecmds.c | 41 ++++++++++++++++++-
src/backend/commands/vacuum.c | 7 ++--
src/include/catalog/partition.h | 1 +
src/include/catalog/pg_class.h | 1 +
src/include/catalog/toasting.h | 1 +
src/include/commands/tablecmds.h | 1 +
.../expected/cluster-conflict-partition.out | 6 ++-
src/test/regress/expected/cluster.out | 4 +-
src/test/regress/expected/vacuum.out | 18 --------
14 files changed, 143 insertions(+), 41 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 79ccddce55..9a4caa769a 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -119,6 +119,28 @@ get_partition_parent_worker(Relation inhRel, Oid relid, bool *detach_pending)
return result;
}
+/*
+ * get_partition_root
+ * Obtain the partitioned table of a given relation
+ *
+ * Note: Because this function assumes that the realtion whose OID is passed as
+ * an argument and each ancestor will have precisely one parent, it should only
+ * be called when it is known that the relation is a partition.
+ */
+Oid
+get_partition_root(Oid relid)
+{
+ List *ancestors = get_partition_ancestors(relid);
+ Oid root = InvalidOid;
+
+ if (list_length(ancestors) > 0)
+ root = llast_oid(ancestors);
+
+ list_free(ancestors);
+
+ return root;
+}
+
/*
* get_partition_ancestors
* Obtain ancestors of given relation
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 9bc10729b0..f9d264e1e6 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -14,6 +14,7 @@
*/
#include "postgres.h"
+#include "access/genam.h"
#include "access/heapam.h"
#include "access/toast_compression.h"
#include "access/xact.h"
@@ -32,6 +33,7 @@
#include "nodes/makefuncs.h"
#include "storage/lock.h"
#include "utils/builtins.h"
+#include "utils/fmgroids.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -413,3 +415,33 @@ needs_toast_table(Relation rel)
/* Otherwise, let the AM decide. */
return table_relation_needs_toast_table(rel);
}
+
+/*
+ * Get the main relation for the given TOAST table.
+ */
+Oid
+get_toast_parent(Oid relid)
+{
+ Relation rel;
+ SysScanDesc scan;
+ ScanKeyData key[1];
+ Oid result = InvalidOid;
+ HeapTuple tuple;
+
+ rel = table_open(RelationRelationId, AccessShareLock);
+
+ ScanKeyInit(&key[0],
+ Anum_pg_class_reltoastrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(relid));
+
+ scan = systable_beginscan(rel, ClassToastRelidIndexId, true, NULL, 1, key);
+ tuple = systable_getnext(scan);
+ if (HeapTupleIsValid(tuple))
+ result = ((Form_pg_class) GETSTRUCT(tuple))->oid;
+ systable_endscan(scan);
+
+ table_close(rel, AccessShareLock);
+
+ return result;
+}
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8966b75bd1..78b2cd62df 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
/*---------------------------------------------------------------------------
@@ -366,8 +367,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
if (recheck)
{
/* Check that the user still has privileges for the relation */
- if (!object_ownercheck(RelationRelationId, tableOid, save_userid) &&
- pg_class_aclcheck(tableOid, save_userid, ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
goto out;
@@ -1646,8 +1646,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
index = (Form_pg_index) GETSTRUCT(indexTuple);
- if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()) &&
- pg_class_aclcheck(index->indrelid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1695,12 +1694,11 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
if (get_rel_relkind(indexrelid) != RELKIND_INDEX)
continue;
- /* Silently skip partitions which the user has no access to. */
- if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
- (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
- IsSharedRelation(relid)))
- continue;
+ /*
+ * We already checked that the user has privileges to CLUSTER the
+ * partitioned table when we locked it earlier, so there's no need to
+ * check the privileges again here.
+ */
/* Use a permanent memory context for the result list */
old_context = MemoryContextSwitchTo(cluster_context);
@@ -1715,3 +1713,20 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
return rtcs;
}
+
+/*
+ * Return whether userid has privileges to CLUSTER relid. If not, this
+ * function emits a WARNING.
+ */
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK ||
+ has_parent_privs(relid, userid, ACL_MAINTAIN))
+ return true;
+
+ ereport(WARNING,
+ (errmsg("permission denied to cluster \"%s\", skipping it",
+ get_rel_name(relid))));
+ return false;
+}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 7dc1aca8fe..3739e05401 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2796,9 +2796,9 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
/* Check permissions */
table_oid = IndexGetRelation(relId, true);
- if (!object_ownercheck(RelationRelationId, relId, GetUserId()) &&
- OidIsValid(table_oid) &&
- pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (OidIsValid(table_oid) &&
+ pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
+ !has_parent_privs(table_oid, GetUserId(), ACL_MAINTAIN))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX,
relation->relname);
@@ -3016,8 +3016,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
* 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_parent_privs(relid, GetUserId(), ACL_MAINTAIN))
continue;
/*
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index e294efc67c..28c75bc093 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,9 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid)
aclresult = pg_class_aclcheck(reloid, userid, aclmask);
+ if (aclresult != ACLCHECK_OK &&
+ has_parent_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 56dc995713..60e0edb455 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16918,12 +16918,49 @@ 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_parent_privs(relId, GetUserId(), ACL_MAINTAIN))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE,
relation->relname);
}
+/*
+ * If relid is a partition, return whether userid has any of the privileges
+ * specified in acl on its partitioned table.
+ *
+ * If relid is a TOAST table, return whether userid has any of the privileges
+ * specified in acl on its main relation.
+ */
+bool
+has_parent_privs(Oid relid, Oid userid, AclMode acl)
+{
+ /*
+ * If this is a partition, check the permissions on its partitioned table.
+ */
+ if (get_rel_relispartition(relid))
+ {
+ Oid root = get_partition_root(relid);
+
+ if (OidIsValid(root) &&
+ pg_class_aclcheck(root, userid, acl) == ACLCHECK_OK)
+ return true;
+ }
+
+ /*
+ * If this is a TOAST table, check the permissions on the main relation.
+ */
+ if (get_rel_relkind(relid) == RELKIND_TOASTVALUE)
+ {
+ Oid parent = get_toast_parent(relid);
+
+ if (OidIsValid(parent) &&
+ pg_class_aclcheck(parent, 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 293b84bbca..9360564945 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"
@@ -568,9 +569,9 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
* - the role owns the current database and the relation is not shared
* - the role has been granted the MAINTAIN privilege on the relation
*/
- 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_parent_privs(relid, GetUserId(), ACL_MAINTAIN))
return true;
relname = NameStr(reltuple->relname);
diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h
index b29df6690c..41144abf3f 100644
--- a/src/include/catalog/partition.h
+++ b/src/include/catalog/partition.h
@@ -20,6 +20,7 @@
#define HASH_PARTITION_SEED UINT64CONST(0x7A5B22367996DCFD)
extern Oid get_partition_parent(Oid relid, bool even_if_detached);
+extern Oid get_partition_root(Oid relid);
extern List *get_partition_ancestors(Oid relid);
extern Oid index_get_partition(Relation partition, Oid indexId);
extern List *map_partition_varattnos(List *expr, int fromrel_varno,
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index e1f4eefa22..f8f2efc2a7 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -155,6 +155,7 @@ typedef FormData_pg_class *Form_pg_class;
DECLARE_UNIQUE_INDEX_PKEY(pg_class_oid_index, 2662, ClassOidIndexId, on pg_class using btree(oid oid_ops));
DECLARE_UNIQUE_INDEX(pg_class_relname_nsp_index, 2663, ClassNameNspIndexId, on pg_class using btree(relname name_ops, relnamespace oid_ops));
DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, ClassTblspcRelfilenodeIndexId, on pg_class using btree(reltablespace oid_ops, relfilenode oid_ops));
+DECLARE_INDEX(pg_class_reltoastrelid_index, 2173, ClassToastRelidIndexId, on pg_class using btree(reltoastrelid oid_ops));
#ifdef EXPOSE_TO_CLIENT_CODE
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index 51e3ba7b69..2afdec5c4a 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -26,5 +26,6 @@ extern void AlterTableCreateToastTable(Oid relOid, Datum reloptions,
LOCKMODE lockmode);
extern void BootstrapToastTable(char *relName,
Oid toastOid, Oid toastIndexOid);
+extern Oid get_toast_parent(Oid relid);
#endif /* TOASTING_H */
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 07eac9a26c..959ae53aa8 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_parent_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; <waiting ...>
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; <waiting ...>
step s1_commit: COMMIT;
+step s2_cluster: <... completed>
step s2_reset: RESET ROLE;
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 542c2e098c..181b55d180 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -353,6 +353,8 @@ INSERT INTO clstr_3 VALUES (1);
-- has not been clustered
SET SESSION AUTHORIZATION regress_clstr_user;
CLUSTER;
+WARNING: permission denied to cluster "pg_toast_826", skipping it
+WARNING: permission denied to cluster "clstr_2", skipping it
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 0035d158b7..41ff4ba9cf 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -347,20 +347,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;
@@ -389,26 +383,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;
--
2.25.1
