Horton

Re: Explain buffers wrong counter with parallel plans

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, "Jonathan S. Katz" <jonathan.katz@excoventures.com>, Alvaro Herrera <alvherre@2ndquadrant.com>, Adrien Nayrat <adrien.nayrat@anayrat.info>, PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Date: 2018-08-03T08:39:23Z
Lists: pgsql-hackers

Attachments

On Thu, Aug 2, 2018 at 11:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Aug 2, 2018 at 5:41 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> I have created three patches (a) move InstrStartParallelQuery from its
>> original location so that we perform it just before ExecutorRun (b)
>> patch to fix the gather stats by calling shutdown at appropriate place
>> and allow stats collection in ExecShutdownNode (c) Probit calling
>> ExecShutdownNode if there is a possibility of backward scans (I have
>> done some basic tests with this patch, if we decide to proceed with
>> it, then some more verification and testing would be required).
>>
>> I think we should commit first two patches as that fixes the problem
>> being discussed in this thread and then do some additional
>> verification for the third patch (mentioned in option c).  I can
>> understand if people want to commit the third patch before the second
>> patch, so let me know what you guys think.
>
> I'm happy with the first two patches.
>

Thanks.  I have pushed those two patches.

>  In the third one, I don't think
> "See ExecLimit" is a good thing to put a comment like this, because
> it's too hard to find the comment to which it refers, and because
> future commits are likely to edit or remove that comment without
> noticing the references to it from elsewhere.  Instead I would just
> write, in all three places, /* If we know we won't need to back up, we
> can release resources at this point. */ or something like that.
>

Okay, I have changed the comment as per your suggestion in the
attached patch.  I will do some more testing/verification of this
patch and will commit over the weekend or on Monday if everything is
fine.

I have noticed that part of the comment atop ExecShutdownNode is now
redundant.  See attached edit_comments_shutdown_node_v1, let me know
if you think otherwise.


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

Commits

  1. Adjust comment atop ExecShutdownNode.

  2. Prohibit shutting down resources if there is a possibility of back up.

  3. Fix buffer usage stats for parallel nodes.

  4. Match the buffer usage tracking for leader and worker backends.

  5. Improve division of labor between execParallel.c and nodeGather[Merge].c.