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

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