From 2ae2377fab3f8eebc4a45f7e762566a02e0df71d Mon Sep 17 00:00:00 2001 From: Justin Pryzby 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