Re: Parallel seq. plan is not coming against inheritance or partition table

On Sun, Mar 5, 2017 at 9:41 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> RCA:
>> ====
>> From "Replace min_parallel_relation_size with two new GUCs" commit
>> onwards, we are not assigning parallel workers for the child rel with
>> zero heap pages. This means we won't be able to create a partial
>> append path as this requires all the child rels within an Append Node
>> to have a partial path.
>
> Right, but OTOH, if we assign parallel workers by default, then it is
> quite possible that it would result in much worse plans. Consider a
> case where partition hierarchy has 1000 partitions and only one of
> them is big enough to allow parallel workers. Now in this case, with
> your proposed fix it will try to scan all the partitions in parallel
> workers which I think can easily result in bad performance. I think
> the right way to make such plans parallel is by using Parallel Append
> node (https://commitfest.postgresql.org/13/987/). Alternatively, if
> you want to force parallelism in cases like the one you have shown in
> example, you can use Alter Table .. Set (parallel_workers = 1).

Well, I think this is a bug in
51ee6f3160d2e1515ed6197594bda67eb99dc2cc. The commit message doesn't
say anything about changing any behavior, just about renaming GUCs,
and I don't remember any discussion about changing the behavior
either, and the comment still implies that we have the old behavior
when we really don't, and parallel append isn't committed yet.

I think the problem here is that compute_parallel_worker() thinks it
can use 0 as a sentinel value that means "ignore this", but it can't
really, because a heap can actually contain 0 pages. Here's a patch
which does the following:

- changes the sentinel value to be -1
- adjusts the test so that a value of -1 is ignored when determining
whether the relation is too small for parallelism
- updates a comment that is out-of-date now that this is no longer
part of the seqscan logic specifically
- moves some variables to narrower scopes
- changes the function parameter types to be doubles, because
otherwise the call in cost_index() has an overflow hazard

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company