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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [COMMITTERS] pgsql: Simplify plpgsql's check for simple expressions.
Date: 2017-08-15 21:15:27
Message-ID: 29231.1502831727@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

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

Attachment Content-Type Size
disallow-setting-parallelModeNeeded-for-unsafe-plans.patch text/x-diff 1.4 KB

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Robert Haas 2017-08-15 22:08:47 Re: [COMMITTERS] pgsql: Simplify plpgsql's check for simple expressions.
Previous Message Alvaro Herrera 2017-08-15 21:14:37 pgsql: Simplify autovacuum work-item implementation

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2017-08-15 21:17:04 Re: POC: Sharing record typmods between backends
Previous Message Jeff Janes 2017-08-15 21:14:58 Re: Create replication slot in pg_basebackup if requested and not yet present