Re: Explain buffers wrong counter with parallel plans

On Fri, May 4, 2018 at 10:35 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, May 2, 2018 at 11:37 AM, Adrien Nayrat
> <adrien(dot)nayrat(at)anayrat(dot)info> wrote:
>> In 9.6 gather node reports sum of buffers for main process + workers. In 10,
>> gather node only reports buffers from the main process.
>
> Oh, really? Well, that sounds like a bug. Amit seems to think it's
> expected behavior, but I don't know why it should be.
>

The reason why I think the current behavior is okay because it is
coincidental that they were displayed correctly. We have not made any
effort to percolate it to upper nodes. For ex., before that commit
also, it was not being displayed for Gather Merge or Gather with some
kind of node like 'Limit' where we have to stop before reaching the
end of the result.

> The commit
> message makes it sound like it's just refactoring, but in fact it
> seems to have made a significant behavior change that doesn't look
> very desirable.
>

I think it is below part of that commit which has made this
difference, basically shutting down the workers.

if (readerdone)
{
Assert(!tup);
..
if (gatherstate->nreaders == 0)
- {
- ExecShutdownGatherWorkers(gatherstate);
return NULL;
- }

The reason why we were getting different results due to above code is
that because while shutting down workers, we gather the buffer usage
of all workers in 'pgBufferUsage' via InstrAccumParallelQuery and then
upper-level node say Gather in this case would get chance to use that
stats via ExecProcNodeInstr->InstrStopNode. However, this won't be
true in other cases where we need to exit before reaching the end of
results like in 'Limit' node case as in such cases after shutting down
the workers via ExecShutdownNode we won't do InstrStopNode for
upper-level nodes. I think if we want that all the stats being
collected by workers should percolate to Gather and nodes above it,
then we need to somehow ensure that we always shutdown
Gather/GatherMerge before we do InstrStopNode for those nodes.

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