Re: Parallel Append can break run-time partition pruning - Mailing list pgsql-hackers

On Thu, Apr 23, 2020 at 7:37 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Thu, 23 Apr 2020 at 02:37, Amit Langote <amitlangote09@gmail.com> wrote:
> > 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?
>
> I started making another pass over this patch again.  I did the
> renaming you've mentioned plus the other two List variables for the
> subpaths.
>
> I did notice that I had forgotten to properly disable parallel append
> when it was disallowed by the rel's consider_parallel flag.  For us to
> have done something wrong, we'd have needed a parallel safe path in
> the pathlist and also have needed the rel to be consider_parallel ==
> false.  It was easy enough to make up a case for that by sticking a
> parallel restrict function in the target list.  I've fixed that issue
> in the attached and also polished up the comments a little and removed
> the unused variable that I had forgotten about.

Thanks for updating the patch.

> For now. I'd still like to get a bit more confidence that the only
> noticeable change in the outcome here is that we're now pulling up
> sub-Appends in all cases.

Yeah, I would have liked us to have enough confidence to remove the
following text in the header comment of accumulate_append_subpath:

... If the latter is
 * NULL, we don't flatten the path at all (unless it contains only partial
 * paths).

> I've read the code a number of times and
> just can't quite see any room for anything changing.  My tests per
> Robert's case all matched what the previous version did, but I'm still
> only about 93% on this. Given that I'm aiming to fix a bug in master,
> v11 and v12 here, I need to get that confidence level up to about the
> 100% mark.

I think I have managed to convince myself (still < 100% though) that
it's not all that bad that we won't be leaving it up to
add_partial_path() to decide between a Parallel Append whose subpaths
set consist only of partial paths and another whose subpaths set
consists of a mix of partial and non-partial paths.  That's because we
will be building only the latter if that one looks cheaper to begin
with.  If Parallel Append is disabled, there could only ever be one
partial Append path for add_partial_path() to consider, one whose
subpaths set consists only of partial paths.

On the patch itself:

+        * We also build a set of paths for each child by trying to use the
+        * cheapest partial path, or the cheapest parallel safe normal path
+        * either when that is cheaper, or if parallel append is not allowed.
         */
-       if (pa_subpaths_valid)
+       if (parallel_subpaths_valid)
        {

In the comment above ", or parallel append is not allowed" should be "
provided parallel append is allowed".  Or how about writing it as
follows:

        /*
         * Add the child's cheapest partial path, or if parallel append is
         * allowed, its cheapest parallel safe normal path if that one is
         * cheaper, to the partial Append path we are constructing for the
         * parent.
         */

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com