Date: Thu, 15 Dec 2022 20:57:00 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
	Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@postgresql.org
Subject: Re: fix and document CLUSTER privileges
Message-ID: <20221216045700.GA804311@nathanxps13>
References: <20221207223924.GA4182184@nathanxps13>
 <20221208022559.GA27893@telsasoft.com>
 <20221208041313.GA216874@nathanxps13>
 <b4d634cf-d893-f0f1-74ce-07675317575c@dunslane.net>
 <20221208181324.GA4385@nathanxps13>
 <CA+TgmoaWhAjTP3a9xjA2P=xkF-RV9UOYT11JVWgQq5A8AB9ZrA@mail.gmail.com>
 <20221214173435.GA690225@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="Qxx1br4bt0+wmkIi"
Content-Disposition: inline
In-Reply-To: <20221214173435.GA690225@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/20221216045700.GA804311%40nathanxps13>
Precedence: bulk


--Qxx1br4bt0+wmkIi
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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).

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--Qxx1br4bt0+wmkIi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="fix_cluster_privs_v3.patch"

diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 145101e6a5..9ca66cd2ee 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 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
@@ -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;
 
 		/* 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)))
+		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;
+}

--Qxx1br4bt0+wmkIi--