On Fri, Dec 8, 2017 at 9:33 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Dec 8, 2017 at 12:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Dec 6, 2017 at 1:05 AM, Amit Kapila <amit.kapila16@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