Date: Thu, 31 Mar 2022 09:10:53 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Michael Paquier <michael@paquier.xyz>,
	=?utf-8?B?5p2O5p2wKOaFjui/vSk=?= <adger.lj@alibaba-inc.com>,
	pgsql-hackers@lists.postgresql.org,
	=?utf-8?B?5pu+5paH5peMKOS5ieS7jik=?= <wenjing.zwj@alibaba-inc.com>,
	Zhihong Yu <zyu@yugabyte.com>
Subject: Re: CLUSTER on partitioned index
Message-ID: <20220331141053.GN28503@telsasoft.com>
References: <20220223184732.GN9008@telsasoft.com>
 <YkTC31yd7wnjn3Dd@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YkTC31yd7wnjn3Dd@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220331141053.GN28503%40telsasoft.com>
Precedence: bulk

On Wed, Mar 30, 2022 at 10:51:43PM +0200, Alvaro Herrera wrote:
> On 2022-Feb-23, Justin Pryzby wrote:
> 
> > I hope that Alvaro will comment on the simplified patches.  If multiple people
> > think the patch isn't worth it, feel free to close it.  But I don't see how
> > complexity could be the reason.
> 
> I gave your patch a look and it seems a reasonable thing to do.  Maybe
> not terribly useful in most cases, but there may be some cases for which
> it is.  I found some part of it a bit repetitive, so I moved things
> around a bit.  What do think about this?

Thanks for looking at it.

The changes to finish_heap_swap() and get_tables_to_cluster() are superfluous,
right ?

I think this comment is worth preserving (it'd be okay if it lived in the
commit message).
-                        * Expand partitioned relations for CLUSTER (the corresponding
-                        * thing for VACUUM FULL happens in and around expand_vacuum_rel()

+       if (rel != NULL) In this case, maybe it should Assert() that it's
relkind=p (mostly for purposes of self-documentation).

+    partition of the specified partitioned index (which must be specified).
This is my own language, but now seems repetitive.  I think the parenthetical
part should be a separate sentance: "For partitioned indexes, the index may not
be omitted.".

Otherwise looks ok.

diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index b3463ae5c46..fbc090cd0b0 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -199,7 +199,8 @@ CLUSTER [VERBOSE]
 
    <para>
     Clustering a partitioned table clusters each of its partitions using the
-    partition of the specified partitioned index (which must be specified).
+    partition of the specified partitioned index.  When clustering a
+    partitioned table, the index may not be omitted.
    </para>
 
  </refsect1>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8417cbdb67f..412147f05bc 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -231,6 +231,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
 	params.options |= CLUOPT_RECHECK;
 	if (rel != NULL)
 	{
+		Assert (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
 		check_index_is_clusterable(rel, indexOid, true, AccessShareLock);
 		rtcs = get_tables_to_cluster_partitioned(cluster_context, indexOid);
 
@@ -451,6 +452,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
 	}
 
 	Assert(OldHeap->rd_rel->relkind == RELKIND_RELATION ||
+		   OldHeap->rd_rel->relkind == RELKIND_TOASTVALUE ||
 		   OldHeap->rd_rel->relkind == RELKIND_MATVIEW);
 
 	/*
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 3f2758d13f6..6cf18c8d321 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -305,6 +305,8 @@ WHERE pg_class.oid=indexrelid
 ---------
 (0 rows)
 
+-- Verify that toast is clusterable
+CLUSTER pg_toast.pg_toast_826 USING pg_toast_826_index;
 -- Verify that clustering all tables does in fact cluster the right ones
 CREATE USER regress_clstr_user;
 CREATE TABLE clstr_1 (a INT PRIMARY KEY);
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 74118993a82..ae27c35f65d 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -104,6 +104,9 @@ WHERE pg_class.oid=indexrelid
 	AND pg_class_2.relname = 'clstr_tst'
 	AND indisclustered;
 
+-- Verify that toast is clusterable
+CLUSTER pg_toast.pg_toast_826 USING pg_toast_826_index;
+
 -- Verify that clustering all tables does in fact cluster the right ones
 CREATE USER regress_clstr_user;
 CREATE TABLE clstr_1 (a INT PRIMARY KEY);