Horton

Thread

Commits

  1. Attempt to clarify comments related to force_parallel_mode.

  2. Make simpler-simple-expressions code cope with a Gather plan.

  3. Simplify plpgsql's check for simple expressions.

  1. pgsql: Simplify plpgsql's check for simple expressions.

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-15T16:28:46Z

    Simplify plpgsql's check for simple expressions.

plpgsql wants to recognize expressions that it can execute directly
via ExecEvalExpr() instead of going through the full SPI machinery.
Originally the test for this consisted of recursively groveling through
the post-planning expression tree to see if it contained only nodes that
plpgsql recognized as safe.  That was a major maintenance headache, since
it required updating plpgsql every time we added any kind of expression
node.  It was also kind of expensive, so over time we added various
pre-planning checks to try to short-circuit having to do that.
Robert Haas pointed out that as of the SRF-processing changes in v10,
particularly the addition of Query.hasTargetSRFs, there really isn't
any reason to make the recursive scan at all: the initial checks cover
everything we really care about.  We do have to make sure that those
checks agree with what inline_function() considers, so that inlining
of a function that formerly wasn't inlined can't cause an expression
considered simple to become non-simple.

Hence, delete the recursive function exec_simple_check_node(), and tweak
those other tests to more exactly agree with inline_function().  Adjust
some comments and function naming to match.

Discussion: https://postgr.es/m/CA+TgmoZGZpwdEV2FQWaVxA_qZXsQE1DAS5Fu8fwxXDNvfndiUQ@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/00418c61244138bd8ac2de58076a1d0dd4f539f3

Modified Files
--------------
src/backend/optimizer/util/clauses.c |   5 +
src/pl/plpgsql/src/pl_exec.c         | 387 +++++------------------------------
2 files changed, 53 insertions(+), 339 deletions(-)

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-15T18:34:58Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> Simplify plpgsql's check for simple expressions.
> ...
> https://git.postgresql.org/pg/commitdiff/00418c61244138bd8ac2de58076a1d0dd4f539f3

The buildfarm members that are running force_parallel_mode = regress
are not happy with this.  Apparently, even a trivial SELECT <expression>
can be turned into a Gather plan if force_parallel_mode says so.

I assume (haven't looked) that I could hack the plpgsql code to prevent
generating a parallel plan when it's decided the command is a simple
SELECT.  But I wonder whether that's the right place to fix it.  Does
it ever make sense to parallelize a plan that can't possibly benefit?
IOW I am not sure that we should carry force_parallel_mode this far.

			regards, tom lane

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

    Robert Haas <robertmhaas@gmail.com> — 2017-08-15T19:15:39Z

    On Tue, Aug 15, 2017 at 2:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Simplify plpgsql's check for simple expressions.
>> ...
>> https://git.postgresql.org/pg/commitdiff/00418c61244138bd8ac2de58076a1d0dd4f539f3
>
> The buildfarm members that are running force_parallel_mode = regress
> are not happy with this.  Apparently, even a trivial SELECT <expression>
> can be turned into a Gather plan if force_parallel_mode says so.
>
> I assume (haven't looked) that I could hack the plpgsql code to prevent
> generating a parallel plan when it's decided the command is a simple
> SELECT.  But I wonder whether that's the right place to fix it.  Does
> it ever make sense to parallelize a plan that can't possibly benefit?
> IOW I am not sure that we should carry force_parallel_mode this far.

Well, I think the point of force_parallel_mode is to try running
things in workers when it's legal but not necessarily beneficial, so
I'd be disinclined to nerf this too much, but it might be OK to nerf
it a little bit.  Off-hand, I'd say the obvious options are:

(1) teach exec_simple_check_plan() to consider everything non-simple
when force_parallel_mode is not off, or
(2) teach exec_save_simple_expr() to see through a Gather node to the
Result node underneath

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-15T20:08:14Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 15, 2017 at 2:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I assume (haven't looked) that I could hack the plpgsql code to prevent
>> generating a parallel plan when it's decided the command is a simple
>> SELECT.  But I wonder whether that's the right place to fix it.  Does
>> it ever make sense to parallelize a plan that can't possibly benefit?
>> IOW I am not sure that we should carry force_parallel_mode this far.

> Well, I think the point of force_parallel_mode is to try running
> things in workers when it's legal but not necessarily beneficial, so
> I'd be disinclined to nerf this too much, but it might be OK to nerf
> it a little bit.

Fair enough.

> Off-hand, I'd say the obvious options are:
> (1) teach exec_simple_check_plan() to consider everything non-simple
> when force_parallel_mode is not off, or
> (2) teach exec_save_simple_expr() to see through a Gather node to the
> Result node underneath

I had been looking into (3) get plpgsql to drop the CURSOR_OPT_PARALLEL_OK
bit when it decides the query is simple.  But because (a) that bit is set
before we make the test, in current behavior, and (b) there are various
SPI interfaces in the way of changing it later, that approach is looking
pretty messy.

I like your plan (2).  It's not much code and it lends itself to having a
run-time check, rather than just an Assert, that we found a Result node.
That seems like a good idea now that we've found the assumption isn't
bulletproof.  However, do we need to worry about the planner putting some
nontrivial computation into the Gather's tlist instead of the Result?

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-15T21:15:27Z

    I wrote:
> I like your plan (2).  It's not much code and it lends itself to having a
> run-time check, rather than just an Assert, that we found a Result node.
> That seems like a good idea now that we've found the assumption isn't
> bulletproof.  However, do we need to worry about the planner putting some
> nontrivial computation into the Gather's tlist instead of the Result?

I concluded that it'd probably be enough to have an assertion that the
Gather's tlist is trivial, so I made it work that way.

However, while investigating the behavior of force_parallel_mode along
the way to this, I found that standard_planner() contains some fuzzy
thinking about how to set parallelModeNeeded.  It's not necessary or
(IMO) approriate to force that on just because of force_parallel_mode,
so I propose the attached patch, which deletes that stanza in favor of
initializing glob->parallelModeNeeded to just plain false.  The effect of
this will be that parallelModeNeeded is only set true if there's actually
a Gather (or GatherMerge) node somewhere in the plan.  The only case where
that differs from the existing behavior is if the initial checks conclude
that parallelModeOK can be turned on, but then we end up with a
parallel-unsafe plan for some reason or other.  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.  If it failed,
we would not conclude that that was an executor bug.

So I think we should apply and back-patch the below.

			regards, tom lane

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

    Robert Haas <robertmhaas@gmail.com> — 2017-08-15T22:08:47Z

    On Tue, Aug 15, 2017 at 5:15 PM, Tom Lane <tgl@sss.pgh.pa.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.  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.
It lets you find planner bugs (or functions that a user has labelled
incorrectly).

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-15T22:40:42Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 15, 2017 at 5:15 PM, Tom Lane <tgl@sss.pgh.pa.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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-15T23:23:12Z

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

... or, more charitably, it may just date from before standard_planner's
plaster-a-Gather-on-top logic worked the way it does today.  But in
any case I think it's wrong now.

			regards, tom lane

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

    Robert Haas <robertmhaas@gmail.com> — 2017-08-16T00:27:31Z

    On Tue, Aug 15, 2017 at 6:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.)

I'm quite confused here.  What's parallel-unsafe about a MinMaxAgg?
There might be some reason why it's parallel-restricted, but it
shouldn't be parallel-unsafe.

More generally, there is no way for parallelModeOK to go from true to
false after it's initially set.  If there were, it would be a bug,
because we might plan some subquery thinking that parallelModeOK is
true, use a Gather node, and then later plan some other subquery that
changes to parallelModeOK from true to false, making the plan that's
already written in stone no longer valid.  This is exactly why we have
to have max_parallel_hazard() walk the ENTIRE query tree, including
all subqueries, before we get started.

Planning can obviously introduce elements into the query that prevent
parallelism from being used for that part of the query, and the only
thing there is to make sure that such things never make it into a
partial path.  But it can't just decide that parallelism is no longer
allowed *anywhere* in the query.

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-16T15:03:54Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 15, 2017 at 6:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> (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.)

> I'm quite confused here.  What's parallel-unsafe about a MinMaxAgg?
> There might be some reason why it's parallel-restricted, but it
> shouldn't be parallel-unsafe.

Well, it has subplans, so formally I think it's restricted not unsafe
--- but the parallel_safe marking on constructed paths/plans is only
safe/not-safe, not a three-way.  By the time we get back up to
standard_planner() and are considering whether to plaster a Gather on
top, it doesn't really matter whether that subtree is unsafe or merely
restricted; either way, you can't send it to a worker.

> More generally, there is no way for parallelModeOK to go from true to
> false after it's initially set.

Sure, and it's not necessary, because that's just a plan-time flag
indicating whether it's worth the trouble to look for parallel plans.
It is not an indicator that we will or must end up choosing a parallel
plan.  parallelModeNeeded is a different animal: it's a planner output
(which AFAICS is never consulted within the planner, so there's no need
to set it early) telling the executor whether to do
EnterParallelMode/ExitParallelMode.

> If there were, it would be a bug,
> because we might plan some subquery thinking that parallelModeOK is
> true, use a Gather node, and then later plan some other subquery that
> changes to parallelModeOK from true to false, making the plan that's
> already written in stone no longer valid.  This is exactly why we have
> to have max_parallel_hazard() walk the ENTIRE query tree, including
> all subqueries, before we get started.

> Planning can obviously introduce elements into the query that prevent
> parallelism from being used for that part of the query, and the only
> thing there is to make sure that such things never make it into a
> partial path.  But it can't just decide that parallelism is no longer
> allowed *anywhere* in the query.

These statements are true, but none of them seem at all relevant to
my point.

			regards, tom lane

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

    Robert Haas <robertmhaas@gmail.com> — 2017-08-16T15:22:29Z

    On Wed, Aug 16, 2017 at 11:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Aug 15, 2017 at 6:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> (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.)
>
>> I'm quite confused here.  What's parallel-unsafe about a MinMaxAgg?
>> There might be some reason why it's parallel-restricted, but it
>> shouldn't be parallel-unsafe.
>
> Well, it has subplans, so formally I think it's restricted not unsafe
> --- but the parallel_safe marking on constructed paths/plans is only
> safe/not-safe, not a three-way.

True, but when parallel_safe it not set, that means it's not
parallel-safe, so either parallel-restricted or parallel-unsafe.  But
if parallelModeOK is true, then it had better be parallel-restricted,
not parallel-unsafe.  Which in turn means that it had better be
perfectly safe to run the plan under
EnterParallelMode()/ExitParallelMode().  If it's not, then <original
query> UNION ALL <something that makes a Gather> would blow up.

So I still don't see what's wrong here, other than that the comment is
evidently not half clear enough.  I don't think this is the first time
we've been over all this.

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-16T15:56:34Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Aug 16, 2017 at 11:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, it has subplans, so formally I think it's restricted not unsafe
>> --- but the parallel_safe marking on constructed paths/plans is only
>> safe/not-safe, not a three-way.

> True, but when parallel_safe it not set, that means it's not
> parallel-safe, so either parallel-restricted or parallel-unsafe.  But
> if parallelModeOK is true, then it had better be parallel-restricted,
> not parallel-unsafe.

Ah, I see.

> So I still don't see what's wrong here, other than that the comment is
> evidently not half clear enough.

I can get on board with that statement.  Can you draft a better wording?

			regards, tom lane

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

    Robert Haas <robertmhaas@gmail.com> — 2017-08-16T16:17:31Z

    On Wed, Aug 16, 2017 at 11:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So I still don't see what's wrong here, other than that the comment is
>> evidently not half clear enough.
>
> I can get on board with that statement.  Can you draft a better wording?

Here is an attempt.  Feel free to edit.

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-16T16:49:32Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Aug 16, 2017 at 11:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I can get on board with that statement.  Can you draft a better wording?

> Here is an attempt.  Feel free to edit.

I think s/plan/query/ in the last bit would be better.  Perhaps

+	 * However, if force_parallel_mode = on or force_parallel_mode = regress,
+	 * then we impose parallel mode whenever it's safe to do so, even if the
+	 * final plan doesn't use parallelism.  It's not safe to do so if the query
+	 * contains anything parallel-unsafe; parallelModeOK will be false in that
+	 * case.  Otherwise, everything in the query is either parallel-safe or
+	 * parallel-restricted, and in either case it should be OK to impose
+	 * parallel-mode restrictions.  If that ends up breaking something, then
+	 * either some function the user included in the query is incorrectly
+	 * labelled as parallel-safe or parallel-restricted when in reality it's
+	 * parallel-unsafe, or else the query planner itself has a bug.
 	 */

			regards, tom lane

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

    Robert Haas <robertmhaas@gmail.com> — 2017-08-16T16:53:53Z

    On Wed, Aug 16, 2017 at 12:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Aug 16, 2017 at 11:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I can get on board with that statement.  Can you draft a better wording?
>
>> Here is an attempt.  Feel free to edit.
>
> I think s/plan/query/ in the last bit would be better.  Perhaps
>
> +        * However, if force_parallel_mode = on or force_parallel_mode = regress,
> +        * then we impose parallel mode whenever it's safe to do so, even if the
> +        * final plan doesn't use parallelism.  It's not safe to do so if the query
> +        * contains anything parallel-unsafe; parallelModeOK will be false in that
> +        * case.  Otherwise, everything in the query is either parallel-safe or
> +        * parallel-restricted, and in either case it should be OK to impose
> +        * parallel-mode restrictions.  If that ends up breaking something, then
> +        * either some function the user included in the query is incorrectly
> +        * labelled as parallel-safe or parallel-restricted when in reality it's
> +        * parallel-unsafe, or else the query planner itself has a bug.
>          */

Works for me.  I'm happy to phrase this in any way that makes it clear
to you, 'cuz it's already clear to me.  :-)

You want to push something, or should I do it?

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-16T16:57:58Z

    Robert Haas <robertmhaas@gmail.com> writes:
> You want to push something, or should I do it?

Go for it.

			regards, tom lane