On Mon, 20 Jul 2020 11:30:34 +0530
Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Jul 18, 2020 at 7:32 PM Jehan-Guillaume de Rorthais
> <jgdr@dalibo.com> wrote:
[...]
> > The Merge node works correctly because it calls ExecShutdownGatherWorkers as
> > soon as the workers are exhausted from gather_readnext. Because of this,
> > buffers from workers are already accounted and propagated to upper nodes
> > before the recursive call of ExecShutdownNode on each nodes. There's no
> > similar call to ExecShutdownGatherMergeWorkers for Gather Merge. Adding a
> > call to ExecShutdownGatherMergeWorkers in gather_merge_getnext when workers
> > are exhausted seems to fix the issue, but I feel like this is the wrong
> > place to fix this issue.
>
> Why do you think so?
Because the fix seemed too specific to the Gather Merge node alone. This bug
might exist for some other nodes (I didn't look for them) and the trap will
stay for futur ones.
The fix in ExecShutdownNode recursion have a broader impact on all present
and futur nodes.
> I think irrespective of whether we want to call
> ExecShutdownGatherMergeWorkers in gather_merge_getnext (when we know
> we are done aka binaryheap_empty) to fix this particular issue, it is
> better to shutdown the workers as soon as we are done similar to what
> we do for Gather node. It is good to release resources as soon as we
> can.
Indeed. I was focusing on the bug and I didn't thought about that. The patch I
test was really just a quick test. I'll have a closer look at this, but I
suppose this might be considered as a separate commit?
Regards,
