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: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>, "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-02T09:41:39Z
Lists: pgsql-hackers

Attachments

On Thu, Aug 2, 2018 at 10:26 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Aug 2, 2018 at 8:38 AM, Andres Freund <andres@anarazel.de> wrote:
>> Hi,
>>
>> On 2018-08-02 08:21:58 +0530, Amit Kapila wrote:
>>> I think something on the lines what Tom and you are suggesting can be
>>> done with the help of EXEC_FLAG_BACKWARD, but I don't see the need to
>>> do anything for this patch.  The change in nodeLimit.c is any way for
>>> forward scans, so there shouldn't be any need for any other check.
>>
>> I think this is almost a guarantee to introduce bugs in the future. And
>> besides that, as Robert points out, it's essentially an exiting bug for
>> custom scans.  Given that EXEC_FLAG_BACKWARD already exists, why not do
>> the right thing here?
>>
>
> Sure, if we want we can do it in this patch, but this is not the
> problem of this patch.  It is also related to existing usage of
> shutdown callbacks.  I think we can use es_top_eflags in Estate to
> detect it and then call shutdown only if EXEC_FLAG_BACKWARD is not
> set.  I think we should do that as a separate patch either before or
> after this patch rather than conflating that change into this patch.
> IIUC, then Robert also said that we should fix that separately.  I
> will do as per whatever consensus we reach here.
>

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.

-- 
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.