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. > > - 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. > > - 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(). > 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 ? > > This needs a bit more polish tomorrow, but I'm starting to like where > this is going. Comments? Yeah, I think the plan output looks reasonable compact now. Thanks. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
