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(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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