On Tue, 25 Sep 2018 at 14:17, Andres Freund <andres@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