Re: explain analyze output with parallel workers - question about meaning of information for explain.depesz.com

On Wed, Dec 13, 2017 at 9:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Thanks.  I think now we can proceed with
> fix_accum_instr_parallel_workers_v8.patch posted above which will fix
> the original issue and the problem we have found in sort and hash
> nodes.

Committed and back-patched to v10.  While I was doing that, I couldn't
help wondering if this code doesn't also need to be moved:

    /*
     * Next, accumulate buffer usage.  (This must wait for the workers to
     * finish, or we might get incomplete data.)
     */
    for (i = 0; i < nworkers; i++)
        InstrAccumParallelQuery(&pei->buffer_usage[i]);

It seems that it doesn't, because the way that instrumentation data
gets accumulated is via InstrStartParallelQuery and
InstrEndParallelQuery, the latter of which clobbers the entry in the
buffer_usage array rather than adding to it:

void
InstrEndParallelQuery(BufferUsage *result)
{
    memset(result, 0, sizeof(BufferUsage));
    BufferUsageAccumDiff(result, &pgBufferUsage, &save_pgBufferUsage);
}

But we could think about choosing to make that work the same way; that
is, move the code block to ExecParallelCleanup, remove the memset()
call from InstrEndParallelQuery, and change the code that allocates
PARALLEL_KEY_BUFFER_USAGE to initialize the space.  That would make
the handling of this more consistent with what we're now doing for the
instrumentation data, although I can't see that it fixes any live bug.

Thoughts?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company