Re: Shouldn't ExecShutdownNode() be called from standard_ExecutorFinish()?

On Thu, Oct 4, 2018 at 12:33 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> There's a few cases where by the time ExecutorFinish() is called,
> ExecShutdownNode() has not yet been called. As, among other reasons,
> ExecShutdownNode() also collects instrumentation, I think that's
> problematic.
>
> In ExplainOnePlan() we call
>
> /* run cleanup too */
> ExecutorFinish(queryDesc);
>
> and then print the majority of the explain data:
>
> /* Create textual dump of plan tree */
> ExplainPrintPlan(es, queryDesc);
>
> and only then shut down the entire query:
>
> ExecutorEnd(queryDesc);
>
> which seems to mean that if a node hasn't yet been shut down for some
> reason, we'll not have information that's normally collected in
> ExecShutdownNode().
>

Ideally, ExecShutdownNode would have been called by the end of
ExecutorRun phase. Can you tell which particular case you are
bothered about?

> ISTM, we should have a new EState member that runs ExecShutdownNode() if
> in standard_ExecutorEnd() if not already done.
>

Even if it wasn't called before due to any reason, I think the
relevant work should be done via
standard_ExecutorEnd->ExecEndPlan->ExecEndNode. For example, for
gather node, it will call ExecEndGather to perform the necessary work.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com