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

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:
>> I wrote:
>>> This appears to be the fault of commit ab7271677, whose authors I've cc'd:
>>> the stanza starting at about allpaths.c:1672 is bullheadedly creating a
>>> parallel path whether that's allowed or not. Fixing it might be as simple
>>> as adding "rel->consider_parallel" to the conditions there.
>>
>> Oh, and while I'm bitching: the same commit has created this exceedingly
>> dangerous coding pattern in create_append_path:
>>
>> pathnode->subpaths = list_concat(subpaths, partial_subpaths);
>>
>> foreach(l, subpaths)
>> {
>> Path *subpath = (Path *) lfirst(l);
>>
>> Because list_concat() modifies its first argument, this will usually
>> have the effect that the foreach traverses the partial_subpaths as
>> well as the main subpaths. But it's very unclear to the reader whether
>> that's intended or not. Worse, if we had *only* partial subpaths so
>> that subpaths was initially NIL, then the loop would fail to traverse
>> the partial subpaths, resulting in inconsistent behavior.
>>
>> 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.

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