Re: [HACKERS] [POC] Faster processing at Gather node

On Sat, Nov 18, 2017 at 7:23 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Nov 10, 2017 at 8:39 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Nov 10, 2017 at 5:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>> I am seeing the assertion failure as below on executing the above
>>> mentioned Create statement:
>>
>
> - if (!ExecContextForcesOids(&gatherstate->ps, &hasoid))
> + if (!ExecContextForcesOids(outerPlanState(gatherstate), &hasoid))
>   hasoid = false;
>
> Don't we need a similar change in nodeGatherMerge.c (in function
> ExecInitGatherMerge)?
>
>> And here are the other patches again, too.
>>
>
> The 0001* patch doesn't apply, please find the attached rebased
> version which I have used to verify the patch.
>
> Now, along with 0001* and 0002*, 0003-skip-gather-project-v2 looks
> good to me.  I think we can proceed with the commit of 0001*~0003*
> patches unless somebody else has any comments.
>

I see that you have committed 0001* and 0002* patches, so continuing my review.

Review of 0006-remove-memory-leak-protection-v1

> remove-memory-leak-protection-v1.patch removes the memory leak
> protection that Tom installed upon discovering that the original
> version of tqueue.c leaked memory like crazy.  I think that it
> shouldn't do that any more, courtesy of
> 6b65a7fe62e129d5c2b85cd74d6a91d8f7564608.  Assuming that's correct, we
> can avoid a whole lot of tuple copying in Gather Merge and a much more
> modest amount of overhead in Gather.  Since my test case exercised
> Gather Merge, this bought ~400 ms or so.

I think Tom didn't installed memory protection in nodeGatherMerge.c
related to an additional copy of tuple.  I could see it is present in
the original commit of Gather Merge
(355d3993c53ed62c5b53d020648e4fbcfbf5f155).  Tom just avoided applying
heap_copytuple to a null tuple in his commit
04e9678614ec64ad9043174ac99a25b1dc45233a.  I am not sure whether you
are just referring to the protection Tom added in nodeGather.c,  If
so, it is not clear from your mail.

I think we don't need the additional copy of tuple in
nodeGatherMerge.c and your patch seem to be doing the right thing.
However, after your changes, it looks slightly odd that
gather_merge_clear_tuples() is explicitly calling heap_freetuple for
the tuples allocated by tqueue.c, maybe we can add some comment.  It
was much clear before this patch as nodeGatherMerge.c was explicitly
copying the tuples that it is freeing.

Isn't it better to explicitly call gather_merge_clear_tuples in
ExecEndGatherMerge so that we can free the memory for tuples allocated
in this node rather than relying on reset/free of MemoryContext in
which those tuples are allocated?

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