Re: Unneeded parallel safety tests in grouping_planner

(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