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