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

On Thu, Oct 4, 2018 at 12:33 AM Andres Freund  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