Re: Proposal: Progressive explain

> But having said that, I'm not quite sure I understand why you're
> proposing (A) and (B1) as separate alternatives. Changing
> progressive_explain to be a Boolean doesn't seem like it solves the
> problem of needing to wrap ExecProcNode from a signal handler. The
> only thing that seems to solve that problem is to instead do the
> wrapping at the start of the query, which AIUI is (A). So I feel like
> you should do (A) to solve the problem I thought we were talking about
> and (B1) to make things simpler. Am I misunderstanding the trade-offs
> here?

Both (A) and (B1) use the same strategy, which is to wrap at query start.
What
changes is that (A), where we keep the 'explain' option, allows users to
still
see the plan without having to include instrumentation overhead.

But (A) is doomed from the start as the custom wrapper will have custom
logic
that will add some overhead. That is what I was able to avoid with the
current
patch that does wrapping in the timeout handler function.

In (B1) it is ok to have a custom wrapper that does the progressive explain
in fixed internals as that overhead is far less noticeable than the overhead
in the already existing ExecProcNodeInstr wrapper. I tested that.

Notice that the gist of (B1) is already part of the existing patch. If
progressive_explain
is set to 'analyze' I set a wrapper ExecProcNodeInstrExplain at query start.

So I think the way to go is with (B1), where the ExecProcNodeInstrExplain
wrapper will continue to do what it does, but only after
progressive_explain_min_duration
has passed (boolean flag set by the timeout handler function). And I get rid
of the 'explain'.

As you said, visibility on the non instrumented query plan is already a
feature
of auto_explain.

> The benefit of such an approach is that it is doable to change the
> Instrumentation, add a hook now, and develop this extension without a
> rush until it is stable - I think at least the case of parallel
> execution may be enhanced.

Thanks for the input Andrei! The main issue is that progressive explain
touches a bunch of different parts of the code that would require additional
hooks. For example, to adjust the execProcNode wrapper at query start. Also
hook for xact/subxact cleanups, I don't think it exists yet.

The biggest challenge is what lies inside ExplainNode(), an already complex
function that calls several other helper functions. Progressive explain had
to change several parts of it.

The only solution I found back then while thinking about this idea was to
make
ExplainNode itself a hook, and the extension would duplicate the whole code
with
the additional custom logic. And that is definitely not a good idea.

With the new hooks Robert added it is indeed one big step ahead, but we
still need
to deal with instrumentation and other nuances as you said.

I am definitely not the authority here to talk about the best way forward.
If there is an agreement in turning this into an extension, it can be a new
feature in auto_explain.

Rafael.