On 20 June 2018 at 14:28, Amit Khandekar <amitdkhan...@gmail.com> wrote: > On 20 June 2018 at 14:24, Amit Kapila <amit.kapil...@gmail.com> wrote: >> On Mon, Jun 18, 2018 at 3:11 PM, Amit Khandekar <amitdkhan...@gmail.com> >> wrote: >>> 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: >>>>> >>>>> 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. >>> >> >> Did you get a chance to look at it? > > Not yet, but I have planned to do this by tomorrow.
After list_concat, the subpaths no longer has only non-partial paths, which it is supposed to have. So it anyways should not be used in the subsequent code in that function. So I think the following change should be good. - foreach(l, subpaths) + foreach(l, pathnode->subpaths) Attached patch contains the above change. You are right that the partial paths do not need to be used for evaluating the pathnode->path.parallel_safe field. But since we also have the parameterization-related assertion in the same loop, I think it is ok to do both the things in one loop, covering all paths. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
fix_subpaths_issue.patch
Description: Binary data
