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. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
fix_accum_instr_parallel_workers_v6.patch
Description: Binary data
