Re: CLUSTER on partitioned index

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