On 2018-09-26 23:45:35 +0530, Amit Khandekar wrote: > On Tue, 25 Sep 2018 at 14:17, Andres Freund <and...@anarazel.de> wrote: > > > > On 2018-09-18 10:03:02 +0530, Amit Khandekar wrote: > > > Attached v3 patch that does the above change. > > > > Attached is a revised version of that patch. I've changed quite a few > > things: > > - I've reverted the split of "base" and "provider specific" contexts - I > > don't think it really buys us anything here. > > The idea was to have a single estate field that accumulates all the > JIT counters of leader as well as workers. I see that you want to > delay the merging of workers and backend counters until end of query > execution. More points on this in the bottom section.
No, I never want to merge things into the leader's stats. > > - I've reverted the context creation changes - instead of creating a > > context in the leader just to store instrumentation in the worker, > > there's now a new EState->es_jit_combined_instr. > > The context created in the leader was a light context, involving only > the resource owner stuff, and not the provider initialization. I know, but it added a fair bit of infrastructure and complications just for that - and I see very very little reason that that'd ever be a necessary separation. > > > > - That also means worker instrumentation doesn't get folded into the > > leader's instrumentation. > > You mean the worker instrumentation doesn't get folded into the leader > until the query execution end, right ? In the committed code, I see > that now we merge the leader instrumentation into the combined worker > instrumentation in standard_ExecutorEnd(). No, I mean it *never* gets folded into the leader's instrumentation. There's a *separate* instrumentation field, where they do get combined. But that still allows code to print out the leader's stats alone. > > This seems good for the future and for > > extensions - it's not actually "linear" time that's spent doing > > JIT in workers (& leader), as all of that work happens in > > parallel. Being able to disentangle that seems important. > > Ok. Your point is: we should have the backend and workers info stored > in two separate fields, and combine them only when we need it; so that > we will be in a position to show combined workers-only info separately > in the future. From the code, it looks like the es_jit_combined_instr > stores combined workers info not just from a single Gather node, but > all the Gather nodes in the plan. If we want to have separate workers > info, I am not sure if it makes sense in combining workers from two > separate Gather nodes; because these two sets of workers are > unrelated, aren't they ? Well, we now have all the individual stats around in an unmodified manner. We could print both the aggregate and individualized stats. Greetings, Andres Freund
