On Wed, Apr 22, 2020 at 12:22 PM David Rowley <dgrowle...@gmail.com> wrote: > On Tue, 21 Apr 2020 at 15:03, David Rowley <dgrowle...@gmail.com> wrote: > > I wonder if the fix should be more something along the lines of trying > > to merge things do we only generate a single partial path. That way > > we wouldn't be at the mercy of the logic in add_partial_path() to > > accept or reject the path based on the order the paths are added. > > In the attached, I'm trying to solve this by only created 1 partial > Append path in the first place. This path will always try to use the > cheapest partial path, or the cheapest parallel safe path, if parallel > append is allowed and that path is cheaper than the cheapest partial > path. > > I believe the attached gives us what we want and additionally, since > it should now always pullup the sub-Appends, then there's no need to > consider adjusting partitioned_rels based on if the pull-up occurred > or not. Those should now be right in all cases. This should also fix > the run-time pruning issue too since in my original test case it'll > pullup the sub-Append which means that the code added in 8edd0e794 is > no longer going to do anything with it as the top-level Append will > never contain just 1 subpath. > > I'm reasonably certain that this is correct, but I did find it a bit > mind-bending considering all the possible cases, so it could do with > some more eyes on it. I've not really done a final polish of the > comments yet. I'll do that if the patch is looking promising.
Thanks for the patch. It's good to see that unfolded sub-Appends will not occur with the new code structure or one hopes. Also, I am finding it somewhat easier to understand how partial Appends get built due to smaller code footprint after patching. One thing I remain concerned about is that it appears like we are no longer leaving the choice between parallel and non-parallel Append to the cost machinery which is currently the case. AFAICS with patched, as long as parallel Append is enabled and allowed, it will be chosen over a non-parallel Append as the partial path. Regarding the patch, I had been assuming that the "pa" in pa_subpaths_valid stands for "parallel append", so it using the variable as is in the new code structure would be misleading. Maybe, parallel_subpaths_valid? -- Amit Langote EnterpriseDB: http://www.enterprisedb.com
