Re: Proposal: Progressive explain

Hi,

Thanks for the valuable feedback Tomas. I am sending a new version of the
patch
that includes:

- Changed instrumented plan printing based on timeouts instead of sampling.
This works perfectly and benchmarks are promising. So new GUC
progressive_explain_sampe_rate is removed.
- Removed all parts of the code where allocated memory for instrumentation
and explain objects is released as this is done automatically when the
custom memory context is released. Comments added later on.
- Adjusted regression tests expected objects so tests pass.

> OK, I understand why it works like this. But I rather dislike that it
> relies on auto_explain to enable the progressive updates. If we want to
> make this dependent on auto_explain, then most of this could/should be
> moved to auto_explain, and called through a hook. Or something similar.

> If we want to add this to core, then I think the progressive_explain GUC
> should have "off|explain|analyze" values, or a similar way to enable the
> instrumentation and regular updates. But that would probably also need
> to duplicate a number of auto_explain options (e.g. to allow disabling
> timings).

> This seems to be the duplication of some auto_explain parameters that I
> mentioned above. Except that it only duplicates some of them. I'd like
> the ability to disable collecting timing info, which is usually by far
> the main overhead.

I implemented using the current logic with the premise that this new feature
shouldn't change how queries are executed. If a query is not using
EXPLAIN ANALYZE then it shouldn't transparently enable instrumentation,
which will then cause additional overhead. If someone wants an instrumented
run, then it explicitly calls EXPLAIN ANALYZE.

That is the same reasoning for coming up with the GUCs. The only ones
available are progressive_explain_format, progressive_explain_settings
and progressive_explain_verbose, which will only affect the explain
output without changing how the query is executed.

But If we come to an agreement that "off|explain|analyze" values for
progressive_explain GUC is a better approach, changing the logic should
be easy. Now that you mentioned the idea, it does make sense.

> I think the right solution to deal with high cost of timing are
> timeouts. Set a timeout, with a handler that sets a "pending" flag, and
> check the flag before calling ProgressiveExplainUpdate(). See for
> example TransactionTimeoutHandler in postinit.c.

Thanks for that. The new patch sent in this message uses a timeout instead.

> Why do we need a shared hash table?

This is where I am storing the printed plans so other backends can read
from. I saw that as the only option apart from printing to files. Do
you recommend something else?

> Yeah. I was wondering how you're going to deal with this. Wouldn't it be
> cheaper to just use a static variable? I don't think this is called
> recursively, and that'd save the palloc/pfree. Haven't tried and not
> sure if it's worth it.

You mean an instrumented object allocated only once that will be updated
with the counters of the currently processed node? That is also an option.
Maybe it is worth testing considering that it may save millions of
palloc/pfree.
Will give it a go.

> Maybe it'd be better to keep this memory context for the query duration
> (e.g. by adding it to queryDesc), and just reset it before the calls?
> That'd cache the memory, and it shouldn't really use a lot of it, no?

Instead of having a dedicated memory context, use the same context as the
one running the query? This will then require manually freeing the
ExplainEstate
allocated in each ProgressiveExplainPrint() call (I removed in this patch
that
manual allocation that wasn't needed currently) and also the allocated
instrumentation objects (which will no longer be needed if we use a static
variable). This will save memory context creation/release and is definitely
a better option. Will test it.

> Not sure the <current> label is all that useful. It seems misleading at
> best, because it's simply the last node that generated the explain. But
> we might have already moved to a different node. And that node may be
> stuck / very expensive, yet we'll see the plan as seemingly waiting in
> some other node.

Right. The idea here is that, for some specific cases, the execution is
momentarily limited to a subpart of the plan. For example, if a HashJoin
is present that whole set of nodes inside the hash creation need to be
complete before other operations are done. If we see a <current> as part
of that hash creation we know that the query is currently in that region,
regardless of which specific node it is.

This could become a feature to be created as part of an extension with the
help of a new hook inside ExplainNode() if we come to the agreement that
it is not really needed out of the box.

> Thanks for your contribution! Such feature would be very useful.
> I did not look carefully about the implementation. I just wanted to let
> you know someone already posted a similar feature in this thread :
> Maybe it could give you some ideas.

Thanks Adrien! Will have a look.

Regards,

Rafael Castro.