Re: [COMMITTERS] pgsql: Simplify plpgsql's check for simple expressions.

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Aug 15, 2017 at 5:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The idea of the existing
>> code seems to be "let's exercise what happens if the executor does
>> EnterParallelMode/ExitParallelMode around any plan whatsoever, even a
>> parallel-unsafe one"; which seems to me to be bogus as heck.

> I think that is not what the current code is doing. If the plan is
> diagnosed as parallel-unsafe, then parallelModeOK will be false and
> nothing will happen.

No. The case that I'm concerned about is where the initial estimate
of "parallelModeOK" is true, but the planner nevertheless selects
a parallel-unsafe plan --- unsafe for some other reason than that it
already has a Gather in it. That would prevent the code further down
in standard_planner from plastering a Gather on top, but we still end up
labeling the output plan with parallelModeNeeded = true.

Now, you might argue that there should be no case where that initial
estimate has parallelModeOK = true and yet we end up with a
parallel-unsafe plan. I do not think I believe that; that estimate
is supposed to be a conservative estimate, not ironclad exactness.
(In fact, a quick look shows a counterexample: if we pick a MinMaxAgg
path, that's parallel unsafe, but the original query might've been
completely safe.)

> If the plan is actually parallel-unsafe but the
> planner doesn't *think* it's parallel-unsafe, then what you are
> talking about will happen, but that seems to me to be a good thing.

No, you have that backwards. If the planner incorrectly thinks the plan
is parallel-safe then it will forcibly put a Gather on top, and we'll mark
parallelModeNeeded = true due to the existing assignment where that is
done, and then we will detect the unsafety at runtime. The case I'm
worried about is where the planner knows (correctly) that the selected
plan is parallel-unsafe, due to some choice made after the initial
parallelModeOK = true estimate.

> It lets you find planner bugs (or functions that a user has labelled
> incorrectly).

Don't believe this argument either. Certainly we want to be able to
detect incorrectly-labeled-safe functions by turning on
force_parallel_mode; but that will happen anyway, since both the initial
parallelModeOK estimate and the final top_plan->parallel_safe flag will
reflect the false safety labeling. (If there's something else in the plan
that makes it parallel-unsafe overall, then the test cannot reveal the
false function labeling anyway.)

As I said, I think this code is based on fuzzy thinking.

regards, tom lane