On Fri, Dec 8, 2017 at 9:33 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Fri, Dec 8, 2017 at 12:06 AM, Robert Haas <robertmh...@gmail.com> wrote: >> On Wed, Dec 6, 2017 at 1:05 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: >>> Right and seeing that I have prepared the patch (posted above [1]) >>> which fixes it such that it will resemble the non-parallel case. >>> Ideally, it would have obviated the need for my previous patch which >>> got committed as 778e78ae. However, now that is committed, I could >>> think of below options: >>> >>> 1. I shall rebase it atop what is committed and actually, I have done >>> that in the attached patch. I have also prepared a regression test >>> case patch just to show the output with and without the patch. >>> 2. For sort node, we can fix it by having some local_info same as >>> shared_info in sort node and copy the shared_info in that or we could >>> reinstate the pointer to the DSM in ExecSortReInitializeDSM() by >>> looking it up in the TOC as suggested by Thomas. If we go this way, >>> then we need a similar fix for hash node as well. >> >> Well, the patch you've actually attached makes the bug go away by >> removing a net of 53 lines of code. The other approach would probably >> add code. So I am tempted to go with the approach you have here. I >> would probably change the T_HashState and T_SortState cases in >> ExecParallelReInitializeDSM so that they still exist, but just do >> something like this: >> >> case T_HashState: >> case T_SortState: >> /* these nodes have DSM state, but no reinitialization is required */ >> break; >> >> That way, it will be more clear to future readers of this code that >> the lack of a reinitialize function is not an oversight, and the >> compiler should optimize these cases away, merging them with the >> default case. >> > > Okay, I have adjusted the patch accordingly. I have also added a > regression test which should produce the same result across different > runs, see if that looks okay to you, then it is better to add such a > test as well. >
The regression test added by patch needs cleanup at the end which I have added in the attached patch. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
fix_accum_instr_parallel_workers_v7.patch
Description: Binary data
