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

On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <tgl@sss.pgh.pa.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.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com