public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v7 3/7] Propagate changes to indisclustered to child/parents 3+ messages / 3 participants [nested] [flat]
* [PATCH v7 3/7] Propagate changes to indisclustered to child/parents @ 2020-10-07 03:11 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2020-10-07 03:11 UTC (permalink / raw) --- 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 9b6673867c..78c2c2ba72 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); @@ -510,66 +511,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 --AA9g+nFNFPYNJKiL Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0004-Invalidate-parent-indexes.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Remove unnecessary check on set-returning functions in values_lists @ 2024-08-28 14:26 Andrei Lepikhov <[email protected]> 2024-08-29 03:03 ` Re: Remove unnecessary check on set-returning functions in values_lists Tom Lane <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Andrei Lepikhov @ 2024-08-28 14:26 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi, I would like to understand the pull_up_simple_values code a little bit more. Pull-up of simple values was implemented in 2015 by commit f4abd02. In the is_simple_values I see a check on the expression_returns_set() of the RTE values list. But since d43a619 in 2017 the check_srf_call_placement has reported an ERROR in the case of set-returning function inside a VALUES expression. Let's demonstrate: SELECT * FROM (VALUES ((generate_series(1,1E2)))); ERROR: set-returning functions are not allowed in VALUES LINE 1: SELECT * FROM (VALUES ((generate_series(1,1E2)))); I think, the expression_returns_set examination doesn't necessary and we can replace it with an assertion, if needed (see attachment). Am I wrong? -- regards, Andrei Lepikhov From 42dc626cf8d41cecb3f88deb578d9cac63934a9c Mon Sep 17 00:00:00 2001 From: "Andrei V. Lepikhov" <[email protected]> Date: Wed, 28 Aug 2024 16:19:16 +0200 Subject: [PATCH] Remove unnecessary check on set-returning functions in values_lists. Probing the VALUES RTE in the is_simple_values routine it is not necessary because parser has done that before. Just to be paranoid, check it inside the assertion. --- src/backend/optimizer/prep/prepjointree.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index 969e257f70..a465c559e7 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -1783,13 +1783,15 @@ is_simple_values(PlannerInfo *root, RangeTblEntry *rte) * about validity of PHVs for the VALUES' outputs. */ + /* To be paranoid, let's recheck functions inside the VALUES */ + Assert(!expression_returns_set((Node *) rte->values_lists)); + /* - * Don't pull up a VALUES that contains any set-returning or volatile + * Don't pull up a VALUES that contains any volatile * functions. The considerations here are basically identical to the * restrictions on a pull-able subquery's targetlist. */ - if (expression_returns_set((Node *) rte->values_lists) || - contain_volatile_functions((Node *) rte->values_lists)) + if (contain_volatile_functions((Node *) rte->values_lists)) return false; /* -- 2.46.0 Attachments: [text/plain] 0001-Remove-unnecessary-check-on-set-returning-functions-.patch (1.5K, ../../[email protected]/2-0001-Remove-unnecessary-check-on-set-returning-functions-.patch) download | inline diff: From 42dc626cf8d41cecb3f88deb578d9cac63934a9c Mon Sep 17 00:00:00 2001 From: "Andrei V. Lepikhov" <[email protected]> Date: Wed, 28 Aug 2024 16:19:16 +0200 Subject: [PATCH] Remove unnecessary check on set-returning functions in values_lists. Probing the VALUES RTE in the is_simple_values routine it is not necessary because parser has done that before. Just to be paranoid, check it inside the assertion. --- src/backend/optimizer/prep/prepjointree.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index 969e257f70..a465c559e7 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -1783,13 +1783,15 @@ is_simple_values(PlannerInfo *root, RangeTblEntry *rte) * about validity of PHVs for the VALUES' outputs. */ + /* To be paranoid, let's recheck functions inside the VALUES */ + Assert(!expression_returns_set((Node *) rte->values_lists)); + /* - * Don't pull up a VALUES that contains any set-returning or volatile + * Don't pull up a VALUES that contains any volatile * functions. The considerations here are basically identical to the * restrictions on a pull-able subquery's targetlist. */ - if (expression_returns_set((Node *) rte->values_lists) || - contain_volatile_functions((Node *) rte->values_lists)) + if (contain_volatile_functions((Node *) rte->values_lists)) return false; /* -- 2.46.0 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Remove unnecessary check on set-returning functions in values_lists 2024-08-28 14:26 Remove unnecessary check on set-returning functions in values_lists Andrei Lepikhov <[email protected]> @ 2024-08-29 03:03 ` Tom Lane <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Tom Lane @ 2024-08-29 03:03 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Andrei Lepikhov <[email protected]> writes: > I think, the expression_returns_set examination doesn't necessary and we > can replace it with an assertion, if needed (see attachment). I think you may be right that this test is not really necessary given the upstream parser test, but nonetheless I'm not inclined to remove it. The upstream test is very far away in code terms, and there are nearby steps like SQL-function inlining that make it less than 100% obvious that an expression that was SRF-free at parse time still is when we get here. I also don't care for destroying the parallel that the comment mentions to the checks done before pulling up a subquery. I'm reminded of Weinberg’s Law: If builders built buildings the way programmers wrote programs, then the first woodpecker that came along would destroy civilization. Unless there's a demonstrable, nontrivial performance hit from this check, I think we should leave it alone. regards, tom lane ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-08-29 03:03 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-10-07 03:11 [PATCH v7 3/7] Propagate changes to indisclustered to child/parents Justin Pryzby <[email protected]> 2024-08-28 14:26 Remove unnecessary check on set-returning functions in values_lists Andrei Lepikhov <[email protected]> 2024-08-29 03:03 ` Re: Remove unnecessary check on set-returning functions in values_lists Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox