pgsql: Fix some interrelated planner issues with initPlans and Param mu

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-committers(at)postgresql(dot)org
Subject: pgsql: Fix some interrelated planner issues with initPlans and Param mu
Date: 2016-07-02 00:06:13
Message-ID: E1bJ8Rp-0003CQ-Pd@gemulon.postgresql.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers

Fix some interrelated planner issues with initPlans and Param munging.

In commit 68fa28f77 I tried to teach SS_finalize_plan() to cope with
initPlans attached anywhere in the plan tree, by dint of moving its
handling of those into the recursion in finalize_plan(). It turns out that
that doesn't really work: if a lower-level plan node emits an initPlan
output parameter in its targetlist, it's legitimate for upper levels to
reference those Params --- and at the point where this code runs, those
references look just like the Param itself, so finalize_plan() quite
properly rejects them as being in the wrong place. We could lobotomize
the checks enough to allow that, probably, but then it's not clear that
we'd have any meaningful check for misplaced Params at all. What seems
better, at least in the near term, is to tweak standard_planner() a bit
so that initPlans are never placed anywhere but the topmost plan node
for a query level, restoring the behavior that occurred pre-9.6. Possibly
we can do better if this code is ever merged into setrefs.c: then it would
be possible to check a Param's placement only when we'd failed to replace
it with a Var referencing a child plan node's targetlist.

BTW, I'm now suspicious that finalize_plan is doing the wrong thing by
returning the node's allParam rather than extParam to be incorporated
in the parent node's set of used parameters. However, it makes no
difference given that initPlans only appear at top level, so I'll leave
that alone for now.

Another thing that emerged from this is that standard_planner() needs
to check for initPlans before deciding that it's safe to stick a Gather
node on top in force_parallel_mode mode. We previously guarded against
that by deciding the plan wasn't wholePlanParallelSafe if any subplans
had been found, but after commit 5ce5e4a12 it's necessary to have this
substitute test, because path parallel_safe markings don't account for
initPlans. (Normally, we'd have decided the paths weren't safe anyway
due to appearances of SubPlan nodes, Params, or CTE scans somewhere in
the tree --- but it's possible for those all to be optimized away while
initPlans still remain.)

Per fuzz testing by Andreas Seltenreich.

Report: <874m89rw7x(dot)fsf(at)credativ(dot)de>

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/7b67a0a49c19a566ee735f3b156677dab11bd902

Modified Files
--------------
src/backend/optimizer/plan/createplan.c | 7 ++--
src/backend/optimizer/plan/planner.c | 26 +++++++++++--
src/backend/optimizer/plan/subselect.c | 17 ++++++++-
src/test/regress/expected/portals.out | 65 +++++++++++++++++++++++++++++++++
src/test/regress/sql/portals.sql | 16 ++++++++
5 files changed, 122 insertions(+), 9 deletions(-)

Browse pgsql-committers by date

  From Date Subject
Next Message Bruce Momjian 2016-07-02 15:22:44 pgsql: doc: mention dependency on collation libraries
Previous Message Andres Freund 2016-07-01 21:32:45 pgsql: Improve WritebackContextInit() comment and prototype argument na