On Tue, Dec 19, 2017 at 11:37 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 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:
>
Right and that is the reason, I have not touched it in the patch.
> 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.
>
Do you mean to say initialize the first time it is allocated or both
during a first time and during rescans?
> 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.
>
I think the change you are proposing makes sense to me.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com