On Tue, Dec 19, 2017 at 11:37 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Wed, Dec 13, 2017 at 9:18 PM, Amit Kapila <amit.kapil...@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
