Date: Thu, 15 Dec 2022 17:19:26 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Jeff Davis <pgsql@j-davis.com>,
	Pavel Luzanov <p.luzanov@postgrespro.ru>,
	pgsql-hackers@postgresql.org
Subject: Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
Message-ID: <20221216011926.GA771496@nathanxps13>
References: <b19eaf6601d142f96ff4c3c3591f430cd81befbd.camel@j-davis.com>
 <20221214032332.GA671806@nathanxps13>
 <b572d238-0de2-9cad-5f34-4741dc627834@postgrespro.ru>
 <f746293762ae262e321f89153d653173ba759636.camel@j-davis.com>
 <8f7172da-2b58-3bd0-97ae-5126e2a7970c@postgrespro.ru>
 <20221214221140.GA1153@telsasoft.com>
 <cf56dc1a36e5c15e19c4be634038029096d96dab.camel@j-davis.com>
 <b41c0577-8e16-c0fc-0d98-f17155047354@postgrespro.ru>
 <295e86c7aeafb8e2623f8eccdc846855bf2c7e0c.camel@j-davis.com>
 <20221215194813.GK1153@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="W/nzBZO5zC0uMSeA"
Content-Disposition: inline
In-Reply-To: <20221215194813.GK1153@telsasoft.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221216011926.GA771496%40nathanxps13>
Precedence: bulk


--W/nzBZO5zC0uMSeA
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Dec 15, 2022 at 01:48:13PM -0600, Justin Pryzby wrote:
> vacuum initially calls vacuum_is_permitted_for_relation() only for the
> partitioned table, and *later* locks the partition and then checks its
> permissions, which is when the message is output.
> 
> Since v15, cluster calls get_tables_to_cluster_partitioned(), which
> silently discards partitions failing ACL.
> 
> We could change it to emit a message, which would seem to behave like
> vacuum, except that the check is happening earlier, and (unlike vacuum)
> partitions skipped later during CLUOPT_RECHECK wouldn't have any message
> output.
> 
> Or we could change cluster_rel() to output a message when skipping.  But
> these patches hardly touched that function at all.  I suppose we could
> change to emit a message during RECHECK (maybe only in master branch).
> If need be, that could be controlled by a new CLUOPT_*.

Yeah, VACUUM/ANALYZE works differently.  For one, you can specify multiple
relations in the command.  Also, VACUUM/ANALYZE only takes an
AccessShareLock when first assessing permissions (which actually skips
partitions).  CLUSTER immediately takes an AccessExclusiveLock, so the
permissions are checked up front.  This is done "to avoid lock-upgrade
hazard in the single-transaction case," which IIUC is what allows CLUSTER
on a single table to run within a transaction block (unlike VACUUM).  I
don't know if running CLUSTER in a transaction block is important.  IMO we
should consider making CLUSTER look a lot more like VACUUM/ANALYZE in this
regard.  The attached patch adds WARNING messages, but we're still far from
consistency with VACUUM/ANALYZE.

Side note: I see that CLUSTER on a partitioned table is disallowed in a
transaction block, which should probably be added to my documentation patch
[0].

[0] https://commitfest.postgresql.org/41/4054/

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

--W/nzBZO5zC0uMSeA
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="add_cluster_skip_messages.patch"

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8966b75bd1..6d09d1c639 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,11 +1694,8 @@ 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)))
+		/* skip partitions which the user has no access to */
+		if (!cluster_is_permitted_for_relation(relid, GetUserId()))
 			continue;
 
 		/* Use a permanent memory context for the result list */
@@ -1715,3 +1711,21 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
 
 	return rtcs;
 }
+
+/*
+ * Check if the current user has privileges to cluster the relation.  If not,
+ * issue a WARNING log message and return false to let the caller decide what
+ * to do with this relation.
+ */
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+	if (object_ownercheck(RelationRelationId, relid, userid) ||
+		pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK)
+		return true;
+
+	ereport(WARNING,
+			(errmsg("permission denied to cluster \"%s\", skipping it",
+					get_rel_name(relid))));
+	return false;
+}
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 542c2e098c..830b306907 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -352,7 +352,11 @@ 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;
-CLUSTER;
+CLUSTER clstr_1;
+CLUSTER clstr_2;
+ERROR:  must be owner of table clstr_2
+CLUSTER clstr_3;
+ERROR:  there is no previously clustered index for table "clstr_3"
 SELECT * FROM clstr_1 UNION ALL
   SELECT * FROM clstr_2 UNION ALL
   SELECT * FROM clstr_3;
@@ -506,6 +510,7 @@ CREATE TEMP TABLE ptnowner_oldnodes AS
   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";
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 6cb9c926c0..43d44238ac 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;
-CLUSTER;
+CLUSTER clstr_1;
+CLUSTER clstr_2;
+CLUSTER clstr_3;
 SELECT * FROM clstr_1 UNION ALL
   SELECT * FROM clstr_2 UNION ALL
   SELECT * FROM clstr_3;

--W/nzBZO5zC0uMSeA--