Re: Unneeded parallel safety tests in grouping_planner

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.

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