Обсуждение: Unneeded parallel safety tests in grouping_planner

Hi,

Yet another thing I noticed while working on [1] is this in
grouping_planner:

    /*
     * If the input rel is marked consider_parallel and there's nothing
that's
     * not parallel-safe in the LIMIT clause, then the final_rel can be
marked
     * consider_parallel as well.  Note that if the query has rowMarks or is
     * not a SELECT, consider_parallel will be false for every relation
in the
     * query.
     */
    if (current_rel->consider_parallel &&
        is_parallel_safe(root, parse->limitOffset) &&
        is_parallel_safe(root, parse->limitCount))
        final_rel->consider_parallel = true;

If there is a need to add a LIMIT node, we don't consider generating
partial paths for the final relation below (see commit
0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52), so it seems unnecessary
anymore to assess the parallel-safety of the LIMIT and OFFSET clauses.
To save cycles, why not remove those tests from that function like the
attached?

Best regards,
Etsuro Fujita

[1] https://commitfest.postgresql.org/22/1950/

On Wed, Feb 27, 2019 at 7:46 AM Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> Yet another thing I noticed while working on [1] is this in
> grouping_planner:
>
>     /*
>      * If the input rel is marked consider_parallel and there's nothing
> that's
>      * not parallel-safe in the LIMIT clause, then the final_rel can be
> marked
>      * consider_parallel as well.  Note that if the query has rowMarks or is
>      * not a SELECT, consider_parallel will be false for every relation
> in the
>      * query.
>      */
>     if (current_rel->consider_parallel &&
>         is_parallel_safe(root, parse->limitOffset) &&
>         is_parallel_safe(root, parse->limitCount))
>         final_rel->consider_parallel = true;
>
> If there is a need to add a LIMIT node, we don't consider generating
> partial paths for the final relation below (see commit
> 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52), so it seems unnecessary
> anymore to assess the parallel-safety of the LIMIT and OFFSET clauses.
> To save cycles, why not remove those tests from that function like the
> attached?

Because in the future we might want to consider generating
partial_paths in cases where we don't do so today.

I repeatedly made the mistake of believing that I could not bother
setting consider_parallel entirely correctly for one reason or
another, and I've gone through multiple iterations of fixing cases
where I did that and it turned out to cause problems.  I now believe
that we should try to get it right in every case, whether or not we
currently think it's possible for it to matter.  Sometimes it matters
in ways that aren't obvious, and it complicates further development.

I don't think we'd save much by changing this test anyway.  Those
is_parallel_safe() tests aren't entirely free, of course, but they
should be very cheap.

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

(2019/02/28 0:46), Robert Haas wrote:
> On Wed, Feb 27, 2019 at 7:46 AM Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp>  wrote:
>> Yet another thing I noticed while working on [1] is this in
>> grouping_planner:
>>
>>      /*
>>       * If the input rel is marked consider_parallel and there's nothing
>> that's
>>       * not parallel-safe in the LIMIT clause, then the final_rel can be
>> marked
>>       * consider_parallel as well.  Note that if the query has rowMarks or is
>>       * not a SELECT, consider_parallel will be false for every relation
>> in the
>>       * query.
>>       */
>>      if (current_rel->consider_parallel&&
>>          is_parallel_safe(root, parse->limitOffset)&&
>>          is_parallel_safe(root, parse->limitCount))
>>          final_rel->consider_parallel = true;
>>
>> If there is a need to add a LIMIT node, we don't consider generating
>> partial paths for the final relation below (see commit
>> 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52), so it seems unnecessary
>> anymore to assess the parallel-safety of the LIMIT and OFFSET clauses.
>> To save cycles, why not remove those tests from that function like the
>> attached?
>
> Because in the future we might want to consider generating
> partial_paths in cases where we don't do so today.
>
> I repeatedly made the mistake of believing that I could not bother
> setting consider_parallel entirely correctly for one reason or
> another, and I've gone through multiple iterations of fixing cases
> where I did that and it turned out to cause problems.  I now believe
> that we should try to get it right in every case, whether or not we
> currently think it's possible for it to matter.  Sometimes it matters
> in ways that aren't obvious, and it complicates further development.
>
> I don't think we'd save much by changing this test anyway.  Those
> is_parallel_safe() tests aren't entirely free, of course, but they
> should be very cheap.

I got the point.  Thanks for the explanation!

Best regards,
Etsuro Fujita