From f41adc43b9b27c7b8aec8c78297ad8042f5049e7 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Tue, 6 Oct 2020 22:11:12 -0500 Subject: [PATCH v3 3/3] Propogate changes to indisclustered to child/parents --- src/backend/commands/cluster.c | 116 +++++++++++++++++--------- src/backend/commands/indexcmds.c | 20 +++++ src/test/regress/expected/cluster.out | 43 ++++++++++ src/test/regress/sql/cluster.sql | 13 +++ 4 files changed, 154 insertions(+), 38 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 391e018bbd..1e36c47ec6 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -73,6 +73,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); @@ -489,65 +490,104 @@ 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 as clustered, and parents of other + * indexes as unclustered. */ void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal) { - HeapTuple indexTuple; - Form_pg_index indexForm; - Relation pg_index; - ListCell *index; + ListCell *lc, *lc2; + Relation pg_index = table_open(IndexRelationId, RowExclusiveLock); + List *inh = find_all_inheritors(RelationGetRelid(rel), + ShareRowExclusiveLock, NULL); /* - * If the index is already marked clustered, no need to do anything. + * Check each index of the relation and set/clear the bit as needed. + * Iterate over the relation's children rather than the index's chilren + * since we need to unset cluster for indexes on intermediate children, + * too. */ - if (OidIsValid(indexOid)) + + foreach(lc, inh) { - if (get_index_isclustered(indexOid)) - return; + Oid inhrelid = lfirst_oid(lc); + Relation thisrel = table_open(inhrelid, ShareRowExclusiveLock); + List *indexes = RelationGetIndexList(thisrel); + + foreach (lc2, indexes) + { + Oid thisIndexOid = lfirst_oid(lc2); + Oid partoid = index_get_partition(thisrel, indexOid); + + if (thisIndexOid == indexOid || partoid == thisIndexOid) // OidIsValid(partoid)) + { + /* A child of the clustered index should be set clustered, too */ + set_indisclustered(thisIndexOid, true, pg_index); + } + else + { + /* indexes which are not children of the clustered index are set unclustered */ + set_indisclustered(thisIndexOid, false, pg_index); + } + + InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0, + InvalidOid, is_internal); + } + + list_free(indexes); + table_close(thisrel, ShareRowExclusiveLock); } + list_free(inh); + /* - * Check each index of the relation and set/clear the bit as needed. + * When setting an index partition unclustered, also remove + * indisclustered from its parents + * use this instead? get_partition_ancestors() */ - pg_index = table_open(IndexRelationId, RowExclusiveLock); - foreach(index, RelationGetIndexList(rel)) + foreach (lc, RelationGetIndexList(rel)) { - Oid thisIndexOid = lfirst_oid(index); + Oid thisIndexOid = lfirst_oid(lc); - indexTuple = SearchSysCacheCopy1(INDEXRELID, - ObjectIdGetDatum(thisIndexOid)); - if (!HeapTupleIsValid(indexTuple)) - elog(ERROR, "cache lookup failed for index %u", thisIndexOid); - indexForm = (Form_pg_index) GETSTRUCT(indexTuple); + if (thisIndexOid == indexOid) + continue; - /* - * Unset the bit if set. We know it's wrong because we checked this - * earlier. - */ - if (indexForm->indisclustered) + while (get_rel_relispartition(thisIndexOid)) { - indexForm->indisclustered = false; - CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple); + thisIndexOid = get_partition_parent(thisIndexOid); + set_indisclustered(thisIndexOid, false, pg_index); } - 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); - } - - 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 67bd9b12dc..206eb4d656 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" @@ -3773,6 +3775,24 @@ 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 */ + 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..91d0c03055 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -505,6 +505,49 @@ 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.) + +-- Check that only one child is marked clustered after marking clustered on a different parent +ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; +ALTER TABLE clstrpart CLUSTER ON clstrpart_idx_2; +ERROR: index "clstrpart_idx_2" for table "clstrpart" does not exist +\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) CLUSTER + "clstrpart1_idx_2" btree (a) +Number of partitions: 2 (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..635df97257 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -222,6 +222,19 @@ 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 +-- Check that only one child is marked clustered after marking clustered on a different parent +ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; +ALTER TABLE clstrpart CLUSTER ON clstrpart_idx_2; +\d clstrpart1 DROP TABLE clstrpart; -- Test CLUSTER with external tuplesorting -- 2.17.0