public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v7 5/7] Invalidate parent index cluster on attach 3+ messages / 3 participants [nested] [flat]
* [PATCH v7 5/7] Invalidate parent index cluster on attach @ 2020-11-06 01:11 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2020-11-06 01:11 UTC (permalink / raw) --- src/backend/commands/indexcmds.c | 21 +++++++++++++++++++++ src/test/regress/expected/cluster.out | 14 ++++++++++++++ src/test/regress/sql/cluster.sql | 5 +++++ 3 files changed, 40 insertions(+) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 4ca1ffbfa4..cc57c149ed 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -4117,6 +4117,27 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid) /* set relispartition correctly on the partition */ update_relispartition(partRelid, OidIsValid(parentOid)); + /* + * If the attached index is not clustered, invalidate cluster mark on + * any parents + */ + 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 6cba3cc4f9..e7f0889743 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -567,6 +567,20 @@ Indexes: "clstrpart_idx" btree (a) Number of partitions: 3 (Use \d+ to list them.) +-- Check that attaching an unclustered index marks the parent unclustered: +ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; +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: 4 (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 a1d132f288..3c8085c69e 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -239,6 +239,11 @@ CLUSTER clstrpart1 USING clstrpart1_idx_2; ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; ALTER TABLE clstrpart1 SET WITHOUT CLUSTER; \d clstrpart +-- Check that attaching an unclustered index marks the parent unclustered: +ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; +CREATE TABLE clstrpart5 (LIKE clstrpart INCLUDING INDEXES); +ALTER TABLE clstrpart ATTACH PARTITION clstrpart5 FOR VALUES FROM (40)TO(50); +\d clstrpart -- Test CLUSTER with external tuplesorting -- 2.17.0 --AA9g+nFNFPYNJKiL Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0006-Preserve-indisclustered-on-children-of-clustered-.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Clarify planner_hook calling convention @ 2022-01-03 15:59 Tom Lane <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Tom Lane @ 2022-01-03 15:59 UTC (permalink / raw) To: Andrey V. Lepikhov <[email protected]>; +Cc: pgsql-hackers "Andrey V. Lepikhov" <[email protected]> writes: > planner hook is frequently used in monitoring and advising extensions. Yeah. > The call to this hook is implemented in the way, that the > standard_planner routine must be called at least once in the hook's call > chain. > But, as I see in [1], it should allow us "... replace the planner > altogether". > In such situation it haven't sense to call standard_planner at all. That's possible in theory, but who's going to do it in practice? There is a monstrous amount of code you'd have to replace. Moreover, if you felt compelled to do it, it's likely because you are making fundamental changes elsewhere too, which means you are more likely going to end up with a fork not an extension. > But, maybe more simple solution is to describe requirements to such kind > of extensions in the code and documentation (See patch in attachment)? > + * 2. If your extension implements some planning activity, write in the extension > + * docs a requirement to set the extension at the begining of shared libraries list. This advice seems pretty unhelpful. If more than one extension is getting into the planner_hook, they can't all be first. (Also, largely the same issue applies to very many of our other hooks.) regards, tom lane ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Clarify planner_hook calling convention @ 2022-01-04 10:44 Andrey V. Lepikhov <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Andrey V. Lepikhov @ 2022-01-04 10:44 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: pgsql-hackers On 1/3/22 8:59 PM, Tom Lane wrote: > "Andrey V. Lepikhov" <[email protected]> writes: >> planner hook is frequently used in monitoring and advising extensions. > > Yeah. > >> The call to this hook is implemented in the way, that the >> standard_planner routine must be called at least once in the hook's call >> chain. >> But, as I see in [1], it should allow us "... replace the planner >> altogether". >> In such situation it haven't sense to call standard_planner at all. > > That's possible in theory, but who's going to do it in practice? We use it in an extension that freezes a plan for specific parameterized query (using plancache + shared storage) - exactly the same technique as extended query protocol does, but spreading across all backends. As I know, the community doesn't like such features, and we use it in enterprise code only. >> But, maybe more simple solution is to describe requirements to such kind >> of extensions in the code and documentation (See patch in attachment)? >> + * 2. If your extension implements some planning activity, write in the extension >> + * docs a requirement to set the extension at the begining of shared libraries list. > > This advice seems pretty unhelpful. If more than one extension is > getting into the planner_hook, they can't all be first. I want to check planner_hook on startup and log an error if it isn't NULL and give a user an advice how to fix it. I want to legalize this logic, if permissible. > > (Also, largely the same issue applies to very many of our other > hooks.) Agreed. Interference between extensions is a very annoying issue now. -- regards, Andrey Lepikhov Postgres Professional ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-01-04 10:44 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-06 01:11 [PATCH v7 5/7] Invalidate parent index cluster on attach Justin Pryzby <[email protected]> 2022-01-03 15:59 Re: Clarify planner_hook calling convention Tom Lane <[email protected]> 2022-01-04 10:44 ` Re: Clarify planner_hook calling convention Andrey V. Lepikhov <[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