On Wed, Apr 22, 2020 at 12:22 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Tue, 21 Apr 2020 at 15:03, David Rowley <dgrowleyml@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
