Re: Buffers from parallel workers not accumulated to upper nodes with gather merge

Le 20 juillet 2020 11:59:00 GMT+02:00, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> a écrit :
>On Mon, Jul 20, 2020 at 1:29 PM Jehan-Guillaume de Rorthais
><jgdr(at)dalibo(dot)com> wrote:
>>
>> On Mon, 20 Jul 2020 11:30:34 +0530
>> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> > On Sat, Jul 18, 2020 at 7:32 PM Jehan-Guillaume de Rorthais
>> > <jgdr(at)dalibo(dot)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?
>>
>
>Good Question. Initially, I thought we will have it in a same commit,
>but now on again thinking about it, we might want to keep this one for
>the HEAD only and ExecShutdownNode related change in the
>back-branches. BTW, can you please test if the problem exist in
>back-branches, and does your change fix it there as well?

Yes. I'll take care of that later today or tomorrow.

Regards,