Subject: fix and document CLUSTER privileges

From: Nathan Bossart <[email-masked]>
Date: Wed, 7 Dec 2022 14:39:24 -0800

Hi hackers,

While looking into other opportunities for per-table permissions, I noticed
a weird discrepancy in CLUSTER.  When evaluating whether the current user
has permission to CLUSTER a table, we ordinarily just check for ownership.
However, the database owner is also allowed to CLUSTER all partitions that
are not shared.  This was added in 3f19e17, and I didn't see any discussion
about it in the corresponding thread [0].

My first instinct is that we should just remove the database ownership
check, which is what I've done in the attached patch.  I don't see any
strong reason to complicate matters with special
database-owner-but-not-shared checks like other commands (e.g., VACUUM).
But perhaps we should do so just for consistency's sake.  Thoughts?

It was also noted elsewhere [1] that the privilege requirements for CLUSTER
are not documented.  The attached patch adds such documentation.

[0] https://postgr.es/m/20220411140609.GF26620%40telsasoft.com
[1] https://postgr.es/m/661148f4-c7f1-dec1-2bc8-29f3bd58e242%40postgrespro.ru

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index c37f4236f1..feb61e6ec2 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -67,7 +67,8 @@ CLUSTER [VERBOSE]
   </para>
 
   <para>
-   <command>CLUSTER</command> without any parameter reclusters all the
+   <command>CLUSTER</command> without a
+   <replaceable class="parameter">table_name</replaceable> reclusters all the
    previously-clustered tables in the current database that the calling user
    owns, or all such tables if called by a superuser.  This
    form of <command>CLUSTER</command> cannot be executed inside a transaction
@@ -132,6 +133,13 @@ CLUSTER [VERBOSE]
  <refsect1>
   <title>Notes</title>
 
+   <para>
+    To cluster a table, one must be the table's owner or a superuser.
+    Database-wide clusters and clusters on partitioned tables will silently
+    skip over any tables that the calling user does not have permission to
+    cluster.
+   </para>
+
    <para>
     In cases where you are accessing single rows randomly
     within a table, the actual order of the data in the
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 07e091bb87..a8a51cc674 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1693,9 +1693,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
 			continue;
 
 		/* Silently skip partitions which the user has no access to. */
-		if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
-			(!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
-			 IsSharedRelation(relid)))
+		if (!object_ownercheck(RelationRelationId, relid, GetUserId()))
 			continue;
 
 		/* Use a permanent memory context for the result list */

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 7 Dec 2022 20:25:59 -0600

> But perhaps we should do so just for consistency's sake.  Thoughts?

Your patch makes it inconsistent with vacuum full, which is strange
because vacuum full calls cluster.

postgres=> VACUUM FULL t;
VACUUM
postgres=> CLUSTER t;
ERROR:  must be owner of table t

BTW, it'd be helpful to copy the relevant parties on this kind of
message, especially if there's a new thread dedicated just to this.

-- 
Justin

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 7 Dec 2022 20:13:13 -0800

> ERROR:  must be owner of table t

This is the existing behavior on HEAD.  I think it has been this way for a
while.  Granted, that doesn't mean it's ideal, but AFAICT it's intentional.

Looking closer, the database ownership check in
get_tables_to_cluster_partitioned() appears to have no meaningful effect.
In this code path, cluster_rel() will always be called with CLUOPT_RECHECK,
and that function only checks for table ownership.  Anything gathered in
get_tables_to_cluster_partitioned() that the user doesn't own will be
skipped.  So, I don't think my patch changes the behavior in any meaningful
way, but I still think it's worthwhile to make the checks consistent.

---

From: Pavel Luzanov <[email-masked]>
Date: Thu, 8 Dec 2022 14:15:58 +0300

> [1] https://postgr.es/m/661148f4-c7f1-dec1-2bc8-29f3bd58e242%40postgrespro.ru

Thanks for the patch. It correctly states the existing behavior.

But perhaps we should wait for the decision in discussion [1] (link above),
since this decision may affect the documentation on the CLUSTER command.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 8 Dec 2022 07:20:28 -0500

> way, but I still think it's worthwhile to make the checks consistent.

We should probably talk about what the privileges should be, though. I
think there's a case to be made that CLUSTER should be governed by the
VACUUM privileges, given how VACUUM FULL is now implemented.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 8 Dec 2022 10:13:24 -0800

> VACUUM privileges, given how VACUUM FULL is now implemented.

Currently, CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX (minus REINDEX
SCHEMA|DATABASE|SYSTEM) require ownership of the relation or superuser.  In
fact, all three use the same RangeVarCallbackOwnsTable() callback function.
My current thinking is that this is good enough.  I don't sense any strong
demand for allowing database owners to run these commands on all non-shared
relations, and there's ongoing work to break out the privileges to GRANT
and predefined roles.  However, I don't have a strong opinion about this.

If we do want to change the permissions model for CLUSTER, it might make
sense to change all three of the aforementioned commands to look more like
VACUUM/ANALYZE.

---

From: Robert Haas <[email-masked]>
Date: Thu, 8 Dec 2022 16:08:40 -0500

On Thu, Dec 8, 2022 at 1:13 PM Nathan Bossart <[email-masked]> wrote:
> and predefined roles.

+1.

I don't see why being the database owner should give you the right to
run a random subset of commands on any table in the database. Tables
have their own system for access privileges; we should use that, or
extend it as required.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 14 Dec 2022 09:34:35 -0800

> have their own system for access privileges; we should use that, or

Here is a rebased version of the patch.

index 145101e6a5..d6b2651657 100644
 
   <para>
-   <command>CLUSTER</command> without any parameter reclusters all the
+   <command>CLUSTER</command> without a
+   <replaceable class="parameter">table_name</replaceable> reclusters all the
    previously-clustered tables in the current database that the calling user
    owns or has the <literal>MAINTAIN</literal> privilege for, or all such tables
    if called by a superuser or a role with privileges of the
@@ -134,6 +135,16 @@ CLUSTER [VERBOSE]
  <refsect1>
   <title>Notes</title>
 
+   <para>
+    To cluster a table, one must have the <literal>MAINTAIN</literal> privilege
+    on the table or be the table's owner, a superuser, or a role with
+    privileges of the
+    <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link>
+    role.  Database-wide clusters and clusters on partitioned tables will
+    silently skip over any tables that the calling user does not have
+    permission to cluster.
+   </para>
+
    <para>
     In cases where you are accessing single rows randomly
     within a table, the actual order of the data in the
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8966b75bd1..8140a90699 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1697,9 +1697,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
 
 		/* 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)))
+			pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
 			continue;

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 15 Dec 2022 20:57:00 -0800

Here is a new version of the patch.  I've moved the privilege checks to a
new function, and I added a note in the docs about clustering partitioned
tables in a transaction block (it's not allowed).

index 145101e6a5..9ca66cd2ee 100644
 
   <para>
 
+   <para>
+    To cluster a table, one must have the <literal>MAINTAIN</literal> privilege
+    on the table or be the table's owner, a superuser, or a role with
+    privileges of the
+    <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link>
+    role.  Database-wide clusters and clusters on partitioned tables will
+    silently skip over any tables that the calling user does not have
+    permission to cluster.
+   </para>
+
    <para>
     In cases where you are accessing single rows randomly
     within a table, the actual order of the data in the
@@ -202,7 +213,8 @@ CLUSTER [VERBOSE]
    <para>
     Clustering a partitioned table clusters each of its partitions using the
     partition of the specified partitioned index.  When clustering a partitioned
-    table, the index may not be omitted.
+    table, the index may not be omitted.  <command>CLUSTER</command> on a
+    partitioned table cannot be executed inside a transaction block.
    </para>
 
  </refsect1>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8966b75bd1..14328abfa6 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 */
@@ -1696,10 +1695,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
 			continue;
 
+		if (!cluster_is_permitted_for_relation(relid, GetUserId()))
 			continue;
 
 		/* Use a permanent memory context for the result list */
@@ -1715,3 +1711,10 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
 
 	return rtcs;
 }
+
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+	return object_ownercheck(RelationRelationId, relid, userid) ||
+		   pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK;
+}