Forking this thread, since the existing CFs have been closed.
https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
On Tue, Oct 06, 2020 at 01:38:23PM +0900, Michael Paquier wrote:
> On Mon, Oct 05, 2020 at 10:07:33PM -0500, Justin Pryzby wrote:
> > Honestly, I think you're over-thinking and over-engineering indisclustered.
> >
> > If "clusteredness" was something we offered to maintain across DML, I think
> > that might be important to provide stronger guarantees. As it is now, I
> > don't
> > think this patch is worth changing the catalog definition.
>
> Well, this use case is new because we are discussing the relationship
> of indisclustered across multiple transactions for multiple indexes,
> so I'd rather have this discussion than not, and I have learnt
> the hard way with REINDEX that we should care a lot about the
> consistency of partition trees at any step of the operation.
indisclustered is only used as a default for "CLUSTER" (without USING). The
worst thing that can happen if it's "inconsistent" is that "CLUSTER;" clusters
a table on the "old" clustered index (that it was already clustered on), which
is what would've happened before running some command which was interrupted.
> Let's
> imagine a simple example here, take this partition tree: p (parent),
> and two partitions p1 and p2. p has two partitioned indexes i and j,
> indexes also present in p1 and p2 as i1, i2, j1 and j2. Let's assume
> that the user has done a CLUSTER on p USING i that completes, meaning
> that i, i1 and i2 have indisclustered set. Now let's assume that the
> user does a CLUSTER on p USING j this time, and that this command
> fails while processing p2, meaning that indisclustered is set for j1,
> i2, and perhaps i or j depending on what the patch does.
I think the state of "indisclustered" at that point is not critical.
The command failed, and the user can re-run it, or ALTER..SET CLUSTER.
Actually, I think the only inconsistent state is if two indexes are both marked
indisclustered.
I'm attaching a counter-proposal to your catalog change, which preserves
indisclustered on children of clustered, partitioned indexes, and invalidates
indisclustered when attaching unclustered indexes.
Also, I noticed that CREATE TABLE (LIKE.. INCLUDING INDEXES) doesn't preserve
indisclustered, but I can't say that's an issue.
--
Justin
>From dd4588352f99186f28fc666c497f85a87ac11da2 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sun, 7 Jun 2020 16:58:42 -0500
Subject: [PATCH v1 1/3] Implement CLUSTER of partitioned table..
This requires either specification of a partitioned index on which to cluster,
or that an partitioned index was previously set clustered.
TODO: handle DB-WIDE "CLUSTER;" for partitioned tables
new partitions need to inherit indisclustered ?
---
doc/src/sgml/ref/cluster.sgml | 6 +
src/backend/commands/cluster.c | 169 +++++++++++++++++++-------
src/bin/psql/tab-complete.c | 1 +
src/include/nodes/parsenodes.h | 5 +-
src/test/regress/expected/cluster.out | 58 ++++++++-
src/test/regress/sql/cluster.sql | 24 +++-
6 files changed, 209 insertions(+), 54 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index b9450e7366..0476cfff72 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -172,6 +172,12 @@ CLUSTER [VERBOSE]
are periodically reclustered.
</para>
+ <para>
+ Clustering a partitioned table clusters each of its partitions using the
+ index partition of the given partitioned index or (if not specified) the
+ partitioned index marked as clustered.
+ </para>
+
</refsect1>
<refsect1>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 0d647e912c..1db8382a27 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -32,7 +32,9 @@
#include "catalog/index.h"
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
+#include "catalog/partition.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_inherits.h"
#include "catalog/toasting.h"
#include "commands/cluster.h"
#include "commands/progress.h"
@@ -75,6 +77,9 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
TransactionId *pFreezeXid,
MultiXactId *pCutoffMulti);
static List *get_tables_to_cluster(MemoryContext cluster_context);
+static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
+ Oid indexOid);
+static void cluster_multiple_rels(List *rvs, bool isTopLevel, int options);
/*---------------------------------------------------------------------------
@@ -116,7 +121,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
AccessExclusiveLock,
0,
RangeVarCallbackOwnsTable, NULL);
- rel = table_open(tableOid, NoLock);
+ rel = table_open(tableOid, ShareUpdateExclusiveLock);
/*
* Reject clustering a remote temp table ... their local buffer
@@ -127,14 +132,6 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot cluster temporary tables of other sessions")));
- /*
- * Reject clustering a partitioned table.
- */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot cluster a partitioned table")));
-
if (stmt->indexname == NULL)
{
ListCell *index;
@@ -172,8 +169,32 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* close relation, keep lock till commit */
table_close(rel, NoLock);
- /* Do the job. */
- cluster_rel(tableOid, indexOid, stmt->options, isTopLevel);
+ if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+ {
+ /* Do the job. */
+ cluster_rel(tableOid, indexOid, stmt->options, isTopLevel);
+ }
+ else
+ {
+ List *rvs;
+ MemoryContext cluster_context;
+
+ /* Refuse to hold strong locks in a user transaction */
+ PreventInTransactionBlock(isTopLevel, "CLUSTER");
+
+ cluster_context = AllocSetContextCreate(PortalContext,
+ "Cluster",
+ ALLOCSET_DEFAULT_SIZES);
+
+ rvs = get_tables_to_cluster_partitioned(cluster_context, indexOid);
+ cluster_multiple_rels(rvs, isTopLevel, stmt->options);
+
+ /* Start a new transaction for the cleanup work. */
+ StartTransactionCommand();
+
+ /* Clean up working storage */
+ MemoryContextDelete(cluster_context);
+ }
}
else
{
@@ -183,7 +204,6 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
*/
MemoryContext cluster_context;
List *rvs;
- ListCell *rv;
/*
* We cannot run this form of CLUSTER inside a user transaction block;
@@ -207,26 +227,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
*/
rvs = get_tables_to_cluster(cluster_context);
- /* Commit to get out of starting transaction */
- PopActiveSnapshot();
- CommitTransactionCommand();
-
- /* Ok, now that we've got them all, cluster them one by one */
- foreach(rv, rvs)
- {
- RelToCluster *rvtc = (RelToCluster *) lfirst(rv);
-
- /* Start a new transaction for each relation. */
- StartTransactionCommand();
- /* functions in indexes may want a snapshot set */
- PushActiveSnapshot(GetTransactionSnapshot());
- /* Do the job. */
- cluster_rel(rvtc->tableOid, rvtc->indexOid,
- stmt->options | CLUOPT_RECHECK,
- isTopLevel);
- PopActiveSnapshot();
- CommitTransactionCommand();
- }
+ cluster_multiple_rels(rvs, isTopLevel, stmt->options | CLUOPT_RECHECK_ISCLUSTERED);
/* Start a new transaction for the cleanup work. */
StartTransactionCommand();
@@ -332,9 +333,10 @@ cluster_rel(Oid tableOid, Oid indexOid, int options, bool isTopLevel)
}
/*
- * Check that the index is still the one with indisclustered set.
+ * Check that the index is still the one with indisclustered set, if needed.
*/
- if (!get_index_isclustered(indexOid))
+ if ((options & CLUOPT_RECHECK_ISCLUSTERED) != 0 &&
+ !get_index_isclustered(indexOid))
{
relation_close(OldHeap, AccessExclusiveLock);
pgstat_progress_end_command();
@@ -378,8 +380,13 @@ cluster_rel(Oid tableOid, Oid indexOid, int options, bool isTopLevel)
/* Check heap and index are valid to cluster on */
if (OidIsValid(indexOid))
+ {
check_index_is_clusterable(OldHeap, indexOid, recheck, AccessExclusiveLock);
+ /* Mark the index as clustered */
+ mark_index_clustered(OldHeap, indexOid, true);
+ }
+
/*
* Quietly ignore the request if this is a materialized view which has not
* been populated from its query. No harm is done because there is no data
@@ -395,6 +402,14 @@ cluster_rel(Oid tableOid, Oid indexOid, int options, bool isTopLevel)
return;
}
+ /* For a partitioned rel, we're done. */
+ if (!RELKIND_HAS_STORAGE(get_rel_relkind(tableOid)))
+ {
+ relation_close(OldHeap, AccessExclusiveLock);
+ pgstat_progress_end_command();
+ return;
+ }
+
/*
* All predicate locks on the tuples or pages are about to be made
* invalid, because we move tuples around. Promote them to relation
@@ -463,6 +478,9 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
* the worst consequence of following broken HOT chains would be that we
* might put recently-dead tuples out-of-order in the new table, and there
* is little harm in that.)
+ *
+ * This also refuses to cluster on an "incomplete" partitioned index
+ * created with "ON ONLY".
*/
if (!OldIndex->rd_index->indisvalid)
ereport(ERROR,
@@ -487,12 +505,6 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
Relation pg_index;
ListCell *index;
- /* Disallow applying to a partitioned table */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot mark index clustered in partitioned table")));
-
/*
* If the index is already marked clustered, no need to do anything.
*/
@@ -565,10 +577,6 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool isTopLevel, bool verbose)
TransactionId frozenXid;
MultiXactId cutoffMulti;
- /* Mark the correct index as clustered */
- if (OidIsValid(indexOid))
- mark_index_clustered(OldHeap, indexOid, true);
-
/* Remember info about rel before closing OldHeap */
relpersistence = OldHeap->rd_rel->relpersistence;
is_system_catalog = IsSystemRelation(OldHeap);
@@ -1563,3 +1571,76 @@ get_tables_to_cluster(MemoryContext cluster_context)
return rvs;
}
+
+/*
+ * Return a List of tables and associated index, where each index is a
+ * partition of the given index
+ */
+static List *
+get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
+{
+ List *inhoids;
+ ListCell *lc;
+ List *rvs = NIL;
+ MemoryContext old_context;
+
+ inhoids = find_all_inheritors(indexOid, NoLock, NULL);
+
+ foreach(lc, inhoids)
+ {
+ Oid indexrelid = lfirst_oid(lc);
+ Oid relid = IndexGetRelation(indexrelid, false);
+ RelToCluster *rvtc;
+
+ /*
+ * Partitioned rels are also processed by cluster_rel, to
+ * call check_index_is_clusterable() and mark_index_clustered().
+ */
+
+ /*
+ * We have to build the list in a different memory context so it will
+ * survive the cross-transaction processing
+ */
+ old_context = MemoryContextSwitchTo(cluster_context);
+
+ rvtc = (RelToCluster *) palloc(sizeof(RelToCluster));
+ rvtc->tableOid = relid;
+ rvtc->indexOid = indexrelid;
+ rvs = lappend(rvs, rvtc);
+
+ MemoryContextSwitchTo(old_context);
+ }
+
+ return rvs;
+}
+
+/* Cluster each relation in a separate transaction */
+static void
+cluster_multiple_rels(List *rvs, bool isTopLevel, int options)
+{
+ ListCell *lc;
+
+ /* Commit to get out of starting transaction */
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+
+ /* Ok, now that we've got them all, cluster them one by one */
+ foreach(lc, rvs)
+ {
+ RelToCluster *rvtc = (RelToCluster *) lfirst(lc);
+
+ /* Start a new transaction for each relation. */
+ StartTransactionCommand();
+
+ /* functions in indexes may want a snapshot set */
+ PushActiveSnapshot(GetTransactionSnapshot());
+
+ /* Do the job. */
+ cluster_rel(rvtc->tableOid, rvtc->indexOid,
+ options | CLUOPT_RECHECK,
+ isTopLevel);
+
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+ }
+}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b2b4f1fd4d..c3f080e691 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -587,6 +587,7 @@ static const SchemaQuery Query_for_list_of_clusterables = {
.catname = "pg_catalog.pg_class c",
.selcondition =
"c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
+ CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
CppAsString2(RELKIND_MATVIEW) ")",
.viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
.namespace = "c.relnamespace",
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 60c2f45466..d428a94454 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3202,8 +3202,9 @@ typedef struct AlterSystemStmt
*/
typedef enum ClusterOption
{
- CLUOPT_RECHECK = 1 << 0, /* recheck relation state */
- CLUOPT_VERBOSE = 1 << 1 /* print progress info */
+ CLUOPT_VERBOSE = 1 << 0, /* print progress info */
+ CLUOPT_RECHECK = 1 << 1, /* recheck relation state */
+ CLUOPT_RECHECK_ISCLUSTERED = 1 << 2, /* recheck relation state for indisclustered */
} ClusterOption;
typedef struct ClusterStmt
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index bdae8fe00c..e4448350e7 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -439,14 +439,62 @@ select * from clstr_temp;
drop table clstr_temp;
RESET SESSION AUTHORIZATION;
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
+CREATE TABLE clstrpart3 PARTITION OF clstrpart DEFAULT PARTITION BY RANGE(a);
+CREATE TABLE clstrpart33 PARTITION OF clstrpart3 DEFAULT;
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+CREATE INDEX clstrpart_only_idx ON ONLY clstrpart (a);
+CLUSTER clstrpart USING clstrpart_only_idx; -- fails
+ERROR: cannot cluster on invalid index "clstrpart_only_idx"
+DROP INDEX clstrpart_only_idx;
CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
-ERROR: cannot mark index clustered in partitioned table
+-- Check that clustering sets new relfilenodes:
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
CLUSTER clstrpart USING clstrpart_idx;
-ERROR: cannot cluster a partitioned table
-DROP TABLE clstrpart;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
+SELECT relname, old.relkind, old.relfilenode = new.relfilenode FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname) ORDER BY 1;
+ relname | relkind | ?column?
+-------------+---------+----------
+ clstrpart | p | t
+ clstrpart1 | p | t
+ clstrpart11 | r | f
+ clstrpart12 | p | t
+ clstrpart2 | r | f
+ clstrpart3 | p | t
+ clstrpart33 | r | f
+(7 rows)
+
+-- Check that clustering sets new indisclustered:
+SELECT i.indexrelid::regclass::text, relkind, indisclustered FROM pg_partition_tree('clstrpart_idx'::regclass) AS tree JOIN pg_index i ON i.indexrelid=tree.relid JOIN pg_class c ON c.oid=indexrelid ORDER BY 1;
+ indexrelid | relkind | indisclustered
+-------------------+---------+----------------
+ clstrpart11_a_idx | i | t
+ clstrpart12_a_idx | I | t
+ clstrpart1_a_idx | I | t
+ clstrpart2_a_idx | i | t
+ clstrpart33_a_idx | i | t
+ clstrpart3_a_idx | I | t
+ clstrpart_idx | I | t
+(7 rows)
+
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
+ Partitioned table "public.clstrpart"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart_idx" btree (a) CLUSTER
+Number of partitions: 3 (Use \d+ to list them.)
+
-- Test CLUSTER with external tuplesorting
create table clstr_4 as select * from tenk1;
create index cluster_sort on clstr_4 (hundred, thousand, tenthous);
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 188183647c..22225dc924 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -196,12 +196,30 @@ drop table clstr_temp;
RESET SESSION AUTHORIZATION;
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
+CREATE TABLE clstrpart3 PARTITION OF clstrpart DEFAULT PARTITION BY RANGE(a);
+CREATE TABLE clstrpart33 PARTITION OF clstrpart3 DEFAULT;
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+CREATE INDEX clstrpart_only_idx ON ONLY clstrpart (a);
+CLUSTER clstrpart USING clstrpart_only_idx; -- fails
+DROP INDEX clstrpart_only_idx;
CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+-- Check that clustering sets new relfilenodes:
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
CLUSTER clstrpart USING clstrpart_idx;
-DROP TABLE clstrpart;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
+SELECT relname, old.relkind, old.relfilenode = new.relfilenode FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname) ORDER BY 1;
+-- Check that clustering sets new indisclustered:
+SELECT i.indexrelid::regclass::text, relkind, indisclustered FROM pg_partition_tree('clstrpart_idx'::regclass) AS tree JOIN pg_index i ON i.indexrelid=tree.relid JOIN pg_class c ON c.oid=indexrelid ORDER BY 1;
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
-- Test CLUSTER with external tuplesorting
--
2.17.0
>From 66070ce3398d9e040f139a147a3f2f995ef51975 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Tue, 6 Oct 2020 20:40:18 -0500
Subject: [PATCH v1 2/3] preserve indisclustered on children of clustered,
partitioned indexes
Note, this takes a parentIndex, but that wasn't previously used ...
UpdateIndexRelation(Oid indexoid, Oid heapoid, Oid parentIndexId,
---
src/backend/catalog/index.c | 2 +-
src/test/regress/expected/cluster.out | 11 +++++++++++
src/test/regress/sql/cluster.sql | 3 +++
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 3ce6eb61d5..92ca0927fa 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -601,7 +601,7 @@ UpdateIndexRelation(Oid indexoid,
values[Anum_pg_index_indisprimary - 1] = BoolGetDatum(primary);
values[Anum_pg_index_indisexclusion - 1] = BoolGetDatum(isexclusion);
values[Anum_pg_index_indimmediate - 1] = BoolGetDatum(immediate);
- values[Anum_pg_index_indisclustered - 1] = BoolGetDatum(false);
+ values[Anum_pg_index_indisclustered - 1] = BoolGetDatum(OidIsValid(parentIndexId) && get_index_isclustered(parentIndexId));
values[Anum_pg_index_indisvalid - 1] = BoolGetDatum(isvalid);
values[Anum_pg_index_indcheckxmin - 1] = BoolGetDatum(false);
values[Anum_pg_index_indisready - 1] = BoolGetDatum(isready);
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index e4448350e7..8f245da46d 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -495,6 +495,17 @@ Indexes:
"clstrpart_idx" btree (a) CLUSTER
Number of partitions: 3 (Use \d+ to list them.)
+CREATE TABLE clstrpart4 PARTITION OF clstrpart FOR VALUES FROM (30)TO(40);
+\d clstrpart4
+ Table "public.clstrpart4"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart FOR VALUES FROM (30) TO (40)
+Indexes:
+ "clstrpart4_a_idx" btree (a) CLUSTER
+
+DROP TABLE clstrpart;
-- Test CLUSTER with external tuplesorting
create table clstr_4 as select * from tenk1;
create index cluster_sort on clstr_4 (hundred, thousand, tenthous);
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 22225dc924..b871ab53c3 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -220,6 +220,9 @@ CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitio
CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
\d clstrpart
+CREATE TABLE clstrpart4 PARTITION OF clstrpart FOR VALUES FROM (30)TO(40);
+\d clstrpart4
+DROP TABLE clstrpart;
-- Test CLUSTER with external tuplesorting
--
2.17.0
>From 2ae2377fab3f8eebc4a45f7e762566a02e0df71d Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Tue, 6 Oct 2020 22:11:12 -0500
Subject: [PATCH v1 3/3] Invalidate indisclustered when attaching unclustered
indexes
---
src/backend/commands/cluster.c | 76 +++++++++++++++------------
src/backend/commands/indexcmds.c | 21 ++++++++
src/test/regress/expected/cluster.out | 27 ++++++++++
src/test/regress/sql/cluster.sql | 9 ++++
4 files changed, 99 insertions(+), 34 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 1db8382a27..58a0605482 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -76,6 +76,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool *pSwapToastByContent,
TransactionId *pFreezeXid,
MultiXactId *pCutoffMulti);
+static void set_indisclustered(Oid indexOid, bool isclustered, Relation pg_index);
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
@@ -492,6 +493,32 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
index_close(OldIndex, NoLock);
}
+/*
+ * Helper for mark_index_clustered
+ * Mark a single index as clustered or not.
+ * pg_index is passed by caller to avoid repeatedly re-opening it.
+ */
+static void
+set_indisclustered(Oid indexOid, bool isclustered, Relation pg_index)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ indexTuple = SearchSysCacheCopy1(INDEXRELID,
+ ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ /* this was checked earlier, but let's be real sure */
+ if (isclustered && !indexForm->indisvalid)
+ elog(ERROR, "cannot cluster on invalid index %u", indexOid);
+
+ indexForm->indisclustered = isclustered;
+ CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
+ heap_freetuple(indexTuple);
+}
+
/*
* mark_index_clustered: mark the specified index as the one clustered on
*
@@ -500,20 +527,9 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
void
mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
{
- HeapTuple indexTuple;
- Form_pg_index indexForm;
Relation pg_index;
ListCell *index;
- /*
- * If the index is already marked clustered, no need to do anything.
- */
- if (OidIsValid(indexOid))
- {
- if (get_index_isclustered(indexOid))
- return;
- }
-
/*
* Check each index of the relation and set/clear the bit as needed.
*/
@@ -523,34 +539,26 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
{
Oid thisIndexOid = lfirst_oid(index);
- indexTuple = SearchSysCacheCopy1(INDEXRELID,
- ObjectIdGetDatum(thisIndexOid));
- if (!HeapTupleIsValid(indexTuple))
- elog(ERROR, "cache lookup failed for index %u", thisIndexOid);
- indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
-
- /*
- * Unset the bit if set. We know it's wrong because we checked this
- * earlier.
- */
- if (indexForm->indisclustered)
- {
- indexForm->indisclustered = false;
- CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
- }
- else if (thisIndexOid == indexOid)
+ if (thisIndexOid != indexOid)
{
- /* this was checked earlier, but let's be real sure */
- if (!indexForm->indisvalid)
- elog(ERROR, "cannot cluster on invalid index %u", indexOid);
- indexForm->indisclustered = true;
- CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
+ Oid parentind = thisIndexOid;
+ set_indisclustered(thisIndexOid, false, pg_index);
+
+ /*
+ * When setting a given index as clustered, also remove
+ * indisclustered from all parents of other partitioned indexes
+ */
+ while (get_rel_relispartition(parentind))
+ {
+ parentind = get_partition_parent(parentind);
+ set_indisclustered(parentind, false, pg_index);
+ }
}
+ else
+ set_indisclustered(thisIndexOid, true, pg_index);
InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
InvalidOid, is_internal);
-
- heap_freetuple(indexTuple);
}
table_close(pg_index, RowExclusiveLock);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d76d7a22dd..99b0bf745e 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -26,12 +26,14 @@
#include "catalog/index.h"
#include "catalog/indexing.h"
#include "catalog/pg_am.h"
+#include "catalog/partition.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
#include "catalog/pg_type.h"
+#include "commands/cluster.h"
#include "commands/comment.h"
#include "commands/dbcommands.h"
#include "commands/defrem.h"
@@ -3894,6 +3896,25 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
/* set relispartition correctly on the partition */
update_relispartition(partRelid, OidIsValid(parentOid));
+ /* if the attached index is not clustered, invalidate all parents cluster mark, if any */
+ /* TODO: if the attached index *is* clustered, then invalidate the cluster mark on any *other* index.. */
+ if ((OidIsValid(parentOid) && get_index_isclustered(parentOid)) ||
+ get_index_isclustered(partRelid))
+ {
+ Relation indrel;
+
+ /* Make relispartition visible */
+ CommandCounterIncrement();
+
+ indrel = table_open(IndexGetRelation(partRelid, false),
+ ShareUpdateExclusiveLock);
+ mark_index_clustered(indrel,
+ get_index_isclustered(partRelid) ? partRelid : InvalidOid,
+ true);
+ table_close(indrel, ShareUpdateExclusiveLock);
+
+ }
+
if (fix_dependencies)
{
/*
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 8f245da46d..6e6fa77c4b 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -505,6 +505,33 @@ Partition of: clstrpart FOR VALUES FROM (30) TO (40)
Indexes:
"clstrpart4_a_idx" btree (a) CLUSTER
+-- Check that attaching an unclustered index marks the parent unclustered:
+CREATE TABLE clstrpart5 (LIKE clstrpart INCLUDING INDEXES);
+ALTER TABLE clstrpart ATTACH PARTITION clstrpart5 FOR VALUES FROM (40)TO(50);
+\d clstrpart
+ Partitioned table "public.clstrpart"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart_idx" btree (a)
+Number of partitions: 5 (Use \d+ to list them.)
+
+-- Check that the parent index is marked not clustered after clustering a partition on a different index:
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+CREATE INDEX clstrpart1_idx_2 ON clstrpart1(a);
+CLUSTER clstrpart1 USING clstrpart1_idx_2;
+\d clstrpart
+ Partitioned table "public.clstrpart"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart_idx" btree (a)
+Number of partitions: 5 (Use \d+ to list them.)
+
DROP TABLE clstrpart;
-- Test CLUSTER with external tuplesorting
create table clstr_4 as select * from tenk1;
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index b871ab53c3..640922ede0 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -222,6 +222,15 @@ CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
\d clstrpart
CREATE TABLE clstrpart4 PARTITION OF clstrpart FOR VALUES FROM (30)TO(40);
\d clstrpart4
+-- Check that attaching an unclustered index marks the parent unclustered:
+CREATE TABLE clstrpart5 (LIKE clstrpart INCLUDING INDEXES);
+ALTER TABLE clstrpart ATTACH PARTITION clstrpart5 FOR VALUES FROM (40)TO(50);
+\d clstrpart
+-- Check that the parent index is marked not clustered after clustering a partition on a different index:
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+CREATE INDEX clstrpart1_idx_2 ON clstrpart1(a);
+CLUSTER clstrpart1 USING clstrpart1_idx_2;
+\d clstrpart
DROP TABLE clstrpart;
-- Test CLUSTER with external tuplesorting
--
2.17.0
