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