public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v3 2/3] preserve indisclustered on children of clustered, partitioned indexes 8+ messages / 5 participants [nested] [flat]
* [PATCH v3 2/3] preserve indisclustered on children of clustered, partitioned indexes @ 2020-10-07 01:40 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Justin Pryzby @ 2020-10-07 01:40 UTC (permalink / raw) Note, this takes a parentIndex, but that wasn't previously used ... UpdateIndexRelation(Oid indexoid, Oid heapoid, Oid parentIndexId, --- src/backend/catalog/index.c | 2 +- src/test/regress/expected/cluster.out | 11 +++++++++++ src/test/regress/sql/cluster.sql | 3 +++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 5ec225abe1..55d429971f 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -603,7 +603,7 @@ UpdateIndexRelation(Oid indexoid, values[Anum_pg_index_indisprimary - 1] = BoolGetDatum(primary); values[Anum_pg_index_indisexclusion - 1] = BoolGetDatum(isexclusion); values[Anum_pg_index_indimmediate - 1] = BoolGetDatum(immediate); - values[Anum_pg_index_indisclustered - 1] = BoolGetDatum(false); + values[Anum_pg_index_indisclustered - 1] = BoolGetDatum(OidIsValid(parentIndexId) && get_index_isclustered(parentIndexId)); values[Anum_pg_index_indisvalid - 1] = BoolGetDatum(isvalid); values[Anum_pg_index_indcheckxmin - 1] = BoolGetDatum(false); values[Anum_pg_index_indisready - 1] = BoolGetDatum(isready); diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index e4448350e7..8f245da46d 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -495,6 +495,17 @@ Indexes: "clstrpart_idx" btree (a) CLUSTER Number of partitions: 3 (Use \d+ to list them.) +CREATE TABLE clstrpart4 PARTITION OF clstrpart FOR VALUES FROM (30)TO(40); +\d clstrpart4 + Table "public.clstrpart4" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition of: clstrpart FOR VALUES FROM (30) TO (40) +Indexes: + "clstrpart4_a_idx" btree (a) CLUSTER + +DROP TABLE clstrpart; -- 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 22225dc924..b871ab53c3 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -220,6 +220,9 @@ 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 +CREATE TABLE clstrpart4 PARTITION OF clstrpart FOR VALUES FROM (30)TO(40); +\d clstrpart4 +DROP TABLE clstrpart; -- Test CLUSTER with external tuplesorting -- 2.17.0 --EDJsL2R9iCFAt7IV Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v3-0003-Propogate-changes-to-indisclustered-to-child-pare.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Add parameter jit_warn_above_fraction @ 2022-04-08 13:39 Stephen Frost <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Stephen Frost @ 2022-04-08 13:39 UTC (permalink / raw) To: Magnus Hagander <[email protected]>; +Cc: David Rowley <[email protected]>; Robert Haas <[email protected]>; Julien Rouhaud <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; PostgreSQL Hackers <[email protected]> Greetings, * Magnus Hagander ([email protected]) wrote: > On Fri, Apr 8, 2022 at 2:19 PM David Rowley <[email protected]> wrote: > > On Fri, 8 Apr 2022 at 23:27, Magnus Hagander <[email protected]> wrote: > > > On Wed, Mar 30, 2022 at 3:04 PM Magnus Hagander <[email protected]> > > wrote: > > >> > > >> On Tue, Mar 29, 2022 at 10:06 PM David Rowley <[email protected]> > > wrote: > > >>> > > >>> If we go with this patch, the problem I see here is that the amount > > >>> of work the JIT compiler must do for a given query depends mostly on > > >>> the number of expressions that must be compiled in the query (also to > > >>> a lesser extent jit_inline_above_cost, jit_optimize_above_cost, > > >>> jit_tuple_deforming and jit_expressions). The DBA does not really have > > >>> much control over the number of expressions in the query. All he or > > >>> she can do to get rid of the warning is something like increase > > >>> jit_above_cost. After a few iterations of that, the end result is > > >>> that jit_above_cost is now high enough that JIT no longer triggers > > >>> for, say, that query to that table with 1000 partitions where no > > >>> plan-time pruning takes place. Is that really a good thing? It likely > > >>> means that we just rarely JIT anything at all! > > >> > > >> > > >> I don't agree with the conclusion of that. > > >> > > >> What the parameter would be useful for is to be able to tune those > > costs (or just turn it off) *for that individual query*. That doesn't mean > > you "rarely JIT anything atll", it just means you rarely JIT that > > particular query. > > > > I just struggle to imagine that anyone is going to spend much effort > > tuning a warning parameter per query. I imagine they're far more > > likely to just ramp it up to only catch some high percentile problems > > or just (more likely) just not bother with it. It seems more likely > > that if anyone was to tune anything per query here it would be > > jit_above_cost, since that actually might have an affect on the > > performance of the query, rather than if it spits out some warning > > message or not. ISTM that if the user knows what to set it to per > > query, then there's very little point in having a warning as we'd be > > alerting them to something they already know about. > > I would not expect people to tune the *warning* at a query level. If > anything, then ys, they would tune the either jit_above_cost or just > jit=off. But the idea being you can do that on a per query level instead of > globally. Yeah, exactly, this is about having a busy system and wanting to know which queries are spending a lot of time doing JIT relative to the query time, so that you can go adjust your JIT parameters or possibly disable JIT for those queries (or maybe bring those cases to -hackers and try to help make our costing better). > > I looked in the -general list to see if we could get some common > > explanations to give us an idea of the most common reason for high JIT > > compilation time. It seems that the plans were never simple. [1] seems > > due to a complex plan. I'm basing that off the "Functions: 167". I > > didn't catch the full plan. From what I can tell, [2] seems to be due > > to "lots of empty tables", so assuming the clamping at 1 page is > > causing issues there. I think both of those cases could be resolved > > by building the costing the way I mentioned. I admit that 2 cases is > > not a very large sample size. > > Again, I am very much for improvements of the costing model. This is in no > way intended to be a replacement for that. It's intended to be a stop-gap. Not sure I'd say it's a 'stop-gap' as it's really very similar, imv anyway, to log_min_duration_statement- you want to know what queries are taking a lot of time but you can't log all of them. > What I see much of today are things like > https://dba.stackexchange.com/questions/264955/handling-performance-problems-with-jit-in-postgres-12 > or > https://dev.to/xenatisch/cascade-of-doom-jit-and-how-a-postgres-update-led-to-70-failure-on-a-critic... > > The bottom line is that people end up with recommendations to turn off JIT > globally more or less by default. Because there's no real useful way today > to figure out when it causes problems vs when it helps. Yeah, that's frustrating. > The addition to pg_stat_statements I pushed a short while ago would help > with that. But I think having a warning like this would also be useful. As > a stop-gap measure, yes, but we really don't know when we will have an > improved costing model for it. I hope you're right and that we can have it > by 16, and then I will definitely advocate for removing the warning again > if it works. Having this in pg_stat_statements is certainly helpful but having a warning also is. I don't think we have to address this in only one way. A lot faster to flip this guc and then look in the logs on a busy system than to install pg_stat_statements, restart the cluster once you get permission to do so, and then query it. Thanks, Stephen Attachments: [application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Add parameter jit_warn_above_fraction @ 2022-04-09 04:20 Julien Rouhaud <[email protected]> parent: Stephen Frost <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Julien Rouhaud @ 2022-04-09 04:20 UTC (permalink / raw) To: Stephen Frost <[email protected]>; +Cc: Magnus Hagander <[email protected]>; David Rowley <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Fri, Apr 08, 2022 at 09:39:18AM -0400, Stephen Frost wrote: > > * Magnus Hagander ([email protected]) wrote: > > The addition to pg_stat_statements I pushed a short while ago would help > > with that. But I think having a warning like this would also be useful. As > > a stop-gap measure, yes, but we really don't know when we will have an > > improved costing model for it. I hope you're right and that we can have it > > by 16, and then I will definitely advocate for removing the warning again > > if it works. > > Having this in pg_stat_statements is certainly helpful but having a > warning also is. I don't think we have to address this in only one way. > A lot faster to flip this guc and then look in the logs on a busy system > than to install pg_stat_statements, restart the cluster once you get > permission to do so, and then query it. +1, especially if you otherwise don't really need or want to have pg_stat_statements enabled, as it's far from being free. Sure you could enable it by default with pg_stat_statements.track = none, but that seems a lot more troublesome than just dynamically enabling a GUC, possibly for a short time and/or for a specific database/role. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Add parameter jit_warn_above_fraction @ 2022-04-09 14:42 Tom Lane <[email protected]> parent: Julien Rouhaud <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Tom Lane @ 2022-04-09 14:42 UTC (permalink / raw) To: Julien Rouhaud <[email protected]>; +Cc: Stephen Frost <[email protected]>; Magnus Hagander <[email protected]>; David Rowley <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; PostgreSQL Hackers <[email protected]> Julien Rouhaud <[email protected]> writes: > On Fri, Apr 08, 2022 at 09:39:18AM -0400, Stephen Frost wrote: >> Having this in pg_stat_statements is certainly helpful but having a >> warning also is. I don't think we have to address this in only one way. >> A lot faster to flip this guc and then look in the logs on a busy system >> than to install pg_stat_statements, restart the cluster once you get >> permission to do so, and then query it. > +1, especially if you otherwise don't really need or want to have > pg_stat_statements enabled, as it's far from being free. Sure you could enable > it by default with pg_stat_statements.track = none, but that seems a lot more > troublesome than just dynamically enabling a GUC, possibly for a short time > and/or for a specific database/role. The arguments against the GUC were not about whether it's convenient. They were about whether the information it gives you is actually going to be of any use. Also, good luck with "looking in the logs", because by default WARNING-level messages don't go to the postmaster log. If that's the intended use-case then the message ought to appear at LOG level (which'd also change the desirable name for the GUC). regards, tom lane ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Add parameter jit_warn_above_fraction @ 2022-04-09 15:21 Julien Rouhaud <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Julien Rouhaud @ 2022-04-09 15:21 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Magnus Hagander <[email protected]>; David Rowley <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; PostgreSQL Hackers <[email protected]> On Sat, Apr 09, 2022 at 10:42:12AM -0400, Tom Lane wrote: > > Also, good luck with "looking in the logs", because by default > WARNING-level messages don't go to the postmaster log. If that's > the intended use-case then the message ought to appear at LOG > level (which'd also change the desirable name for the GUC). I must be missing something because by default log_min_messages is WARNING, and AFAICS I do get WARNING-level messages in some vanilla cluster logs, using vanilla .psqlrc. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Add parameter jit_warn_above_fraction @ 2022-04-09 16:31 Tom Lane <[email protected]> parent: Julien Rouhaud <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Tom Lane @ 2022-04-09 16:31 UTC (permalink / raw) To: Julien Rouhaud <[email protected]>; +Cc: Stephen Frost <[email protected]>; Magnus Hagander <[email protected]>; David Rowley <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; PostgreSQL Hackers <[email protected]> Julien Rouhaud <[email protected]> writes: > On Sat, Apr 09, 2022 at 10:42:12AM -0400, Tom Lane wrote: >> Also, good luck with "looking in the logs", because by default >> WARNING-level messages don't go to the postmaster log. > I must be missing something because by default log_min_messages is WARNING, and > AFAICS I do get WARNING-level messages in some vanilla cluster logs, using > vanilla .psqlrc. Ah, sorry, I was looking at the wrong thing namely log_min_error_statement. The point still holds though: if we emit this at level WARNING, what will get logged is just the bare message and not the statement that triggered it. How useful do you think that will be? regards, tom lane ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Add parameter jit_warn_above_fraction @ 2022-04-09 16:43 Julien Rouhaud <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Julien Rouhaud @ 2022-04-09 16:43 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Magnus Hagander <[email protected]>; David Rowley <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; PostgreSQL Hackers <[email protected]> On Sat, Apr 09, 2022 at 12:31:23PM -0400, Tom Lane wrote: > Julien Rouhaud <[email protected]> writes: > > On Sat, Apr 09, 2022 at 10:42:12AM -0400, Tom Lane wrote: > >> Also, good luck with "looking in the logs", because by default > >> WARNING-level messages don't go to the postmaster log. > > > I must be missing something because by default log_min_messages is WARNING, and > > AFAICS I do get WARNING-level messages in some vanilla cluster logs, using > > vanilla .psqlrc. > > Ah, sorry, I was looking at the wrong thing namely > log_min_error_statement. The point still holds though: if we emit this > at level WARNING, what will get logged is just the bare message and not > the statement that triggered it. How useful do you think that will be? Ah right, I did miss that "small detail". And of course I agree, as-is that would be entirely useless and it should be LOG, like the rest of similar features. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Add parameter jit_warn_above_fraction @ 2022-09-15 08:02 Ibrar Ahmed <[email protected]> parent: Julien Rouhaud <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Ibrar Ahmed @ 2022-09-15 08:02 UTC (permalink / raw) To: Magnus Hagander <[email protected]>; PostgreSQL Hackers <[email protected]>; +Cc: Tom Lane <[email protected]>; Stephen Frost <[email protected]>; David Rowley <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Julien Rouhaud <[email protected]> On Sat, Apr 9, 2022 at 8:43 PM Julien Rouhaud <[email protected]> wrote: > On Sat, Apr 09, 2022 at 12:31:23PM -0400, Tom Lane wrote: > > Julien Rouhaud <[email protected]> writes: > > > On Sat, Apr 09, 2022 at 10:42:12AM -0400, Tom Lane wrote: > > >> Also, good luck with "looking in the logs", because by default > > >> WARNING-level messages don't go to the postmaster log. > > > > > I must be missing something because by default log_min_messages is > WARNING, and > > > AFAICS I do get WARNING-level messages in some vanilla cluster logs, > using > > > vanilla .psqlrc. > > > > Ah, sorry, I was looking at the wrong thing namely > > log_min_error_statement. The point still holds though: if we emit this > > at level WARNING, what will get logged is just the bare message and not > > the statement that triggered it. How useful do you think that will be? > > Ah right, I did miss that "small detail". And of course I agree, as-is > that > would be entirely useless and it should be LOG, like the rest of similar > features. > > > The patch does not apply successfully; please rebase the patch. patching file src/backend/utils/misc/guc.c Hunk #1 FAILED at 642. Hunk #2 FAILED at 3943. 2 out of 2 hunks FAILED -- saving rejects to file src/backend/utils/misc/guc.c.rej patching file src/backend/utils/misc/postgresql.conf.sample patching file src/include/jit/jit.h -- Ibrar Ahmed ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2022-09-15 08:02 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-10-07 01:40 [PATCH v3 2/3] preserve indisclustered on children of clustered, partitioned indexes Justin Pryzby <[email protected]> 2022-04-08 13:39 Re: Add parameter jit_warn_above_fraction Stephen Frost <[email protected]> 2022-04-09 04:20 ` Re: Add parameter jit_warn_above_fraction Julien Rouhaud <[email protected]> 2022-04-09 14:42 ` Re: Add parameter jit_warn_above_fraction Tom Lane <[email protected]> 2022-04-09 15:21 ` Re: Add parameter jit_warn_above_fraction Julien Rouhaud <[email protected]> 2022-04-09 16:31 ` Re: Add parameter jit_warn_above_fraction Tom Lane <[email protected]> 2022-04-09 16:43 ` Re: Add parameter jit_warn_above_fraction Julien Rouhaud <[email protected]> 2022-09-15 08:02 ` Re: Add parameter jit_warn_above_fraction Ibrar Ahmed <[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