Horton

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

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Rafia Sabih <rafia.sabih@enterprisedb.com>, PostgreSQL Developers <pgsql-hackers@postgresql.org>
Date: 2017-11-25T15:43:24Z
Lists: pgsql-hackers 
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

Commits

  1. C comment: fix wording about shared memory message queue

  2. shm_mq: Have the receiver set the sender's less frequently.

  3. shm_mq: Reduce spinlock usage.

  4. Remove use of byte-masking macros in record_image_cmp

  5. Remove memory leak protection from Gather and Gather Merge nodes.

  6. Avoid projecting tuples unnecessarily in Gather and Gather Merge.

  7. Tweak use of ExecContextForcesOids by Gather (Merge).

  8. Pass eflags down to parallel workers.

  9. Disable installcheck tests for test_session_hooks

  10. First-draft release notes for 10.1.

  11. Remove TupleDesc remapping logic from tqueue.c.

  12. Avoid memory leaks when a GatherMerge node is rescanned.

  13. Code review for nodeGatherMerge.c.

  14. Add a Gather Merge executor node.

  15. Push scan/join target list beneath Gather when possible.