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