Re: server crashed with TRAP: FailedAssertion("!(!parallel_aware || pathnode->path.parallel_safe)"

On 20 June 2018 at 14:28, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
> On 20 June 2018 at 14:24, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Mon, Jun 18, 2018 at 3:11 PM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
>>> On 16 June 2018 at 10:44, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>> On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>>
>>>>> It looks to me like traversal of the partial subpaths is the right
>>>>> thing here, in which case we should do
>>>>>
>>>>> - foreach(l, subpaths)
>>>>> + foreach(l, pathnode->subpaths)
>>>>>
>>>>> or perhaps better
>>>>>
>>>>> - pathnode->subpaths = list_concat(subpaths, partial_subpaths);
>>>>> + pathnode->subpaths = subpaths = list_concat(subpaths, partial_subpaths);
>>>>>
>>>>> to make the behavior clear and consistent.
>>>>>
>>>>
>>>> I agree with your analysis and proposed change. However, I think in
>>>> practice, it might not lead to any bug as in the loop, we are
>>>> computing parallel_safety and partial_subpaths should be
>>>> parallel_safe.
>>>
>>> Will have a look at this soon.
>>>
>>
>> Did you get a chance to look at it?
>
> Not yet, but I have planned to do this by tomorrow.

After list_concat, the subpaths no longer has only non-partial paths,
which it is supposed to have. So it anyways should not be used in the
subsequent code in that function. So I think the following change
should be good.
- foreach(l, subpaths)
+ foreach(l, pathnode->subpaths)

Attached patch contains the above change.

You are right that the partial paths do not need to be used for
evaluating the pathnode->path.parallel_safe field. But since we also
have the parameterization-related assertion in the same loop, I think
it is ok to do both the things in one loop, covering all paths.

--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company