Re: Proposal: Progressive explain - Mailing list pgsql-hackers

Thanks Robert. Very thorough analysis there.

Things I don't comment on will be fixed without further discussion.

> There is a comment in ExplainOnePlan() that says "Handle progressive
> explain cleanup manually if enabled as it is not called as part of
> ExecutorFinish," but standard_ExecutorFinish does indeed call
> ProgressiveExplainFinish(), so either the comment is misleading or the
> code is wrong.

The comment is misleading. What I meant to say is that queries executed via
EXPLAIN (without analyze) don't call ExecutorFinish, so ProgressiveExplainFinish
isn't called in that context. I will fix the comment.

> Calling ProgressiveExplainTrigger() directly from
> ProgressiveExplainStartupTimeoutHandler() seems extremely scary --
> we've tried hard to make our signal handlers do only very simple
> things like setting a flag, and this one runs around the entire
> PlanState tree modifying Very Important Things. I fear that's going to
> be hard to make robust. Like, what happens if we're going around
> trying to change ExecProcNode pointers while the calling code was also
> going around trying to change ExecProcNode pointers? I can't quite see
> how this won't result in chaos.

Agreed. In that other similar patch to log query plans after a signal is sent
from another session there was the same discussion and concerns.

I don't see another way of doing it here. This patch became 100x more complex
after I added GUC progressive_explain_min_duration, that required changing the
execProcNode wrapper on the fly.

I can see some alternatives here:

A) Use a temporary execProcNode wrapper set at query start here:

/*
 * If instrumentation is required, change the wrapper to one that just
 * does instrumentation.  Otherwise we can dispense with all wrappers and
 * have ExecProcNode() directly call the relevant function from now on.
 */
if (node->instrument)
{
  /*
   * Use wrapper for progressive explains if enabled and the node
   * belongs to the currently tracked query descriptor.
   */
  if (progressive_explain == PROGRESSIVE_EXPLAIN_ANALYZE &&
    ProgressiveExplainIsActive(node->state->query_desc))
    node->ExecProcNode = ExecProcNodeInstrExplain;
  else
    node->ExecProcNode = ExecProcNodeInstr;

This wrapper will have an additional logic that will check if a boolean set
by the timeout function has changed. When that happens the initial progressive
explain setup is done and execProcNode is unwrapped in place. This should be
safe.

The problem here is that all queries would always be using the custom
wrapper until timeout is triggered, and that can add unnecessary overhead.

B) We get rid of the idea of applying progressive explains to non instrumented
runs (which was my original idea). My main use-case here is to see progress
of instrumented runs anyways. For that idea we have 2 possibilities:

B1) Keep implementation as is, with all the existing GUCs to control what
is included in the printed plan. We just change GUC progressive_explain to be
a boolean again. If true, instrumentation will be enabled for the query being
executed and progressive explain will be printed consecutively.

B2) Get rid of all new GUCs I added and change the progressive explain feature
to become an option of the EXPLAIN command. Something like:

EXPLAIN (ANALYZE, PROGRESSIVE) SELECT * FROM ...

(B1) would allow progressive explans in regular queries, but I'm not sure people
would be willing to enable it globally as it adds instrumentation overhead.

What do you think of the options?

Rafael.