On 16 June 2018 at 10:44, Amit Kapila <amit.kapil...@gmail.com> wrote: > 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.
Will have a look at this soon. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company