public inbox for [email protected]
help / color / mirror / Atom feedFrom: Justin Pryzby <[email protected]>
Subject: [PATCH v8 4/8] Propagate changes to indisclustered to child/parents
Date: Tue, 6 Oct 2020 22:11:12 -0500
---
src/backend/commands/cluster.c | 109 ++++++++++++++++----------
src/backend/commands/indexcmds.c | 2 +
src/test/regress/expected/cluster.out | 46 +++++++++++
src/test/regress/sql/cluster.sql | 11 +++
4 files changed, 125 insertions(+), 43 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index cb4fc350c6..5c08f0642e 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -74,6 +74,7 @@ static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose);
static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, 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);
@@ -516,66 +517,88 @@ 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
*
- * With indexOid == InvalidOid, will mark all indexes of rel not-clustered.
+ * With indexOid == InvalidOid, mark all indexes of rel not-clustered.
+ * Otherwise, mark children of the clustered index as clustered, and parents of
+ * other indexes as unclustered.
+ * We wish to maintain the following properties:
+ * 1) Only one index on a relation can be marked clustered at once
+ * 2) If a partitioned index is clustered, then all its children must be
+ * clustered.
*/
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;
- }
+ ListCell *lc, *lc2;
+ List *indexes;
+ Relation pg_index = table_open(IndexRelationId, RowExclusiveLock);
+ List *inh = find_all_inheritors(RelationGetRelid(rel), ShareRowExclusiveLock, NULL);
/*
* Check each index of the relation and set/clear the bit as needed.
+ * Iterate over the relation's children rather than the index's children
+ * since we need to unset cluster for indexes on intermediate children,
+ * too.
*/
- pg_index = table_open(IndexRelationId, RowExclusiveLock);
-
- foreach(index, RelationGetIndexList(rel))
+ foreach(lc, inh)
{
- 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);
+ Oid inhrelid = lfirst_oid(lc);
+ Relation thisrel = table_open(inhrelid, ShareRowExclusiveLock);
- /*
- * Unset the bit if set. We know it's wrong because we checked this
- * earlier.
- */
- if (indexForm->indisclustered)
+ indexes = RelationGetIndexList(thisrel);
+ foreach (lc2, indexes)
{
- indexForm->indisclustered = false;
- CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
- }
- else 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);
- }
+ bool isclustered;
+ Oid thisIndexOid = lfirst_oid(lc2);
+ List *parentoids = get_rel_relispartition(thisIndexOid) ?
+ get_partition_ancestors(thisIndexOid) : NIL;
- InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
- InvalidOid, is_internal);
+ /*
+ * A child of the clustered index must be set clustered;
+ * indexes which are not children of the clustered index are
+ * set unclustered
+ */
+ isclustered = (thisIndexOid == indexOid) ||
+ list_member_oid(parentoids, indexOid);
+ Assert(OidIsValid(indexOid) || !isclustered);
+ set_indisclustered(thisIndexOid, isclustered, pg_index);
+
+ InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
+ InvalidOid, is_internal);
+ }
- heap_freetuple(indexTuple);
+ list_free(indexes);
+ table_close(thisrel, ShareRowExclusiveLock);
}
+ list_free(inh);
table_close(pg_index, RowExclusiveLock);
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 127ba7835d..4ca1ffbfa4 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -25,6 +25,7 @@
#include "catalog/catalog.h"
#include "catalog/index.h"
#include "catalog/indexing.h"
+#include "catalog/partition.h"
#include "catalog/pg_am.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
@@ -32,6 +33,7 @@
#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"
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index c74cfa88cc..1d436dfaae 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -495,6 +495,52 @@ Indexes:
"clstrpart_idx" btree (a) CLUSTER
Number of partitions: 3 (Use \d+ to list them.)
+-- Test that it recurses to grandchildren:
+\d clstrpart33
+ Table "public.clstrpart33"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart3 DEFAULT
+Indexes:
+ "clstrpart33_a_idx" btree (a) CLUSTER
+
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+\d clstrpart33
+ Table "public.clstrpart33"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart3 DEFAULT
+Indexes:
+ "clstrpart33_a_idx" btree (a)
+
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+\d clstrpart33
+ Table "public.clstrpart33"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart3 DEFAULT
+Indexes:
+ "clstrpart33_a_idx" btree (a) CLUSTER
+
+-- Check that only one child is marked clustered after marking clustered on a different parent
+CREATE INDEX clstrpart1_idx_2 ON clstrpart1(a);
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+ALTER TABLE clstrpart1 CLUSTER ON clstrpart1_idx_2;
+\d clstrpart1
+ Partitioned table "public.clstrpart1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart FOR VALUES FROM (1) TO (10)
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart1_a_idx" btree (a)
+ "clstrpart1_idx_2" btree (a) CLUSTER
+Number of partitions: 2 (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 9bcc77695c..0ded2be1ca 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -220,6 +220,17 @@ 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
+-- Test that it recurses to grandchildren:
+\d clstrpart33
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+\d clstrpart33
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+\d clstrpart33
+-- Check that only one child is marked clustered after marking clustered on a different parent
+CREATE INDEX clstrpart1_idx_2 ON clstrpart1(a);
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+ALTER TABLE clstrpart1 CLUSTER ON clstrpart1_idx_2;
+\d clstrpart1
-- Test CLUSTER with external tuplesorting
--
2.17.0
--O5XBE6gyVG5Rl6Rj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0005-Invalidate-parent-indexes.patch"
view thread (100+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v8 4/8] Propagate changes to indisclustered to child/parents
In-Reply-To: <no-message-id-1867142@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox