On Wed, Nov 22, 2017 at 8:36 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> 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.
Yes, that's what I mean. What got done for Gather Merge was motivated
by what Tom did for Gather. Sorry for not expressing the idea more
precisely.
> 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.
OK, I can add a comment about that.
> 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?
Generally relying on reset/free of a MemoryContext is cheaper.
Typically you only want to free manually if the freeing would
otherwise not happen soon enough.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
