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

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

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 407df9a..e407b34 100644
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** standard_planner(Query *parse, int curso
*** 249,254 ****
--- 249,255 ----
      glob->lastPlanNodeId = 0;
      glob->transientPlan = false;
      glob->dependsOnRole = false;
+     glob->parallelModeNeeded = false;

      /*
       * Assess whether it's feasible to use parallel mode for this query. We
*************** standard_planner(Query *parse, int curso
*** 290,307 ****
          glob->parallelModeOK = false;
      }

-     /*
-      * glob->parallelModeNeeded should tell us whether it's necessary to
-      * impose the parallel mode restrictions, but we don't actually want to
-      * impose them unless we choose a parallel plan, so it is normally set
-      * only if a parallel plan is chosen (see create_gather_plan).  That way,
-      * people who mislabel their functions but don't use parallelism anyway
-      * aren't harmed.  But when force_parallel_mode is set, we enable the
-      * restrictions whenever possible for testing purposes.
-      */
-     glob->parallelModeNeeded = glob->parallelModeOK &&
-         (force_parallel_mode != FORCE_PARALLEL_OFF);
-
      /* Determine what fraction of the plan is likely to be scanned */
      if (cursorOptions & CURSOR_OPT_FAST_PLAN)
      {
--- 291,296 ----

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers