On Wed, Nov 22, 2017 at 8:36 AM, Amit Kapila <amit.kapil...@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
