Horton

Re: [POC] Faster processing at Gather node

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>, Rafia Sabih <rafia.sabih@enterprisedb.com>, PostgreSQL Developers <pgsql-hackers@postgresql.org>
Date: 2017-11-09T05:08:22Z
Lists: pgsql-hackers 
On Wed, Nov 8, 2017 at 1:02 AM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> On 2017-11-06 10:56:43 +0530, Amit Kapila wrote:
>> On Sun, Nov 5, 2017 at 6:54 AM, Andres Freund <andres@anarazel.de> wrote
>> > On 2017-11-05 01:05:59 +0100, Robert Haas wrote:
>> >> skip-gather-project-v1.patch does what it says on the tin.  I still
>> >> don't have a test case for this, and I didn't find that it helped very
>> >> much,
>>
>> I am also wondering in which case it can help and I can't think of the
>> case.
>
> I'm confused?  Isn't it fairly obvious that unnecessarily projecting
> at the gather node is wasteful? Obviously depending on the query you'll
> see smaller / bigger gains, but that there's beenfdits should be fairly obvious?
>
>

I agree that there could be benefits depending on the statement.  I
initially thought that we are kind of re-evaluating the expressions in
target list as part of projection even if worker backend has already
done that, but that was not the case and instead, we are deforming the
tuples sent by workers.  Now, I think as a general principle it is a
good idea to delay the deforming as much as possible.

About the patch,


  /*
- * Initialize result tuple type and projection info.
- */
- ExecAssignResultTypeFromTL(&gatherstate->ps);
- ExecAssignProjectionInfo(&gatherstate->ps, NULL);
-

- /*
  * Initialize funnel slot to same tuple descriptor as outer plan.
  */
  if (!ExecContextForcesOids(&gatherstate->ps, &hasoid))
@@ -115,6 +109,12 @@ ExecInitGather(Gather *node, EState *estate, int eflags)
  tupDesc = ExecTypeFromTL(outerNode->targetlist, hasoid);
  ExecSetSlotDescriptor(gatherstate->funnel_slot, tupDesc);

+ /*
+ * Initialize result tuple type and projection info.
+ */
+ ExecAssignResultTypeFromTL(&gatherstate->ps);
+ ExecConditionalAssignProjectionInfo(&gatherstate->ps, tupDesc, OUTER_VAR);
+

This change looks suspicious to me.  I think here we can't use the
tupDesc constructed from targetlist.  One problem, I could see is that
the check for hasOid setting in tlist_matches_tupdesc won't give the
correct answer.   In case of the scan, we use the tuple descriptor
stored in relation descriptor which will allow us to take the right
decision in tlist_matches_tupdesc.  If you try the statement CREATE
TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r'; in
force_parallel_mode=regress, then you can reproduce the problem I am
trying to highlight.


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

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.