Re: fixing consider_parallel for upper planner rels

On Mon, Jun 27, 2016 at 6:04 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 5:28 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 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.
>
> Huh? The final tlist would go with the final_rel, ISTM, not the scan
> relation. Maybe we have some rejiggering to do to make that true, though.

Mumble. You're right that there are two rels involved, but I think
I'm still right about the substance of the problem. I can't tell
whether the remainder of your email concedes that point or whether
we're still in disagreement.

In that example, SELECT pg_backend_pid() FROM pgbench_accounts, we're
first going to form a path for scanning pgbench_accounts. The rel for
pgbench_accounts will be marked parallel_safe because it's just a scan
of a relation outputting some number (possibly 0) of Vars. That rel
becomes the final scan/join rel, and the path or paths for that rel
are parallel-safe. Now, when we apply the final tlist to those paths,
they are no longer parallel-safe. apply_projection_to_path() has got
to realize that.

> You could still save something by writing code along the line of
> if (path->parallel_safe &&
> has_parallel_hazard(...))
> path->parallel_safe = false;
> so as not to run has_parallel_hazard in the case where we already know
> we lost.

I agree, and that does seem worth doing.

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