Re: fixing consider_parallel for upper planner rels

On Mon, Jun 27, 2016 at 5:28 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jun 27, 2016 at 4:49 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> * Not following what you did to apply_projection_to_path, and the comment
>>> therein isn't helping.
>
>> Gee, I wonder why not? :-)
>
>> The basic problem here is that applying a projection to a path can
>> render a formerly parallel-safe path no longer parallel-safe. If we
>> jam a parallel-restricted target list into a formerly parallel-safe
>> path, we'd better also clear path->parallel_safe. Currently,
>> apply_projection_to_path only needs to call has_parallel_hazard for an
>> input which is a GatherPath, which isn't too expensive because most
>> paths are not GatherPaths and if we get one that is, well, we can hope
>> parallel query will win enough during execution to make up for the
>> extra planning cost. But if we want the consider_parallel and
>> parallel_safe flags to be set correctly for all upper rels and paths,
>> it seems that we need to do it always - hence the dismayed comment.
>> Thoughts?
>
> Seems to me that it should generally be the case that consider_parallel
> would already be clear on the parent rel if the tlist isn't parallel safe,
> and if it isn't we probably have a bug elsewhere. If it makes you feel
> better, maybe you could add Assert(!has_parallel_hazard(...)) here?

I don't see that this is true. If someone does SELECT
pg_backend_pid() FROM pgbench_accounts, there's only one RelOptInfo
and nothing to clear consider_parallel for it anywhere else.

I've really been wondering whether there ought to be a separate
UPPERREL_FOO whose only purpose is to handle applying scanjoin_target
to the topmost scan/join rel. Effectively, the current code is trying
to do that transformation in place. We start with a scan/join rel
that emits whatever it naturally emits and then frob all of the paths
and partial paths to emit the scanjoin_target. But this seems quite
cumbersome.

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