Re: BUG #18953: Planner fails to build plan for complex query with LATERAL references - Mailing list pgsql-bugs

I wrote:
> Doing things the way Richard wanted to (ie using the nestloop's
> required_outer) gets past the "failed to assign all NestLoopParams"
> error, but then we get the same "unrecognized node type: 22" error
> as in the first example.

> That error seems considerably nastier to fix.  I think it is a
> pre-existing problem, though I don't currently have an explanation
> why we've not seen it reported before.  What is happening is that
> we pull up a PHV for "c", containing a SubLink for (SELECT true),
> and only some copies of it get put through preprocess_expression,
> which is responsible for converting SubLinks to SubPlans among
> other essential tasks.  It seems that up to now we've always
> managed to use only preprocessed copies in the finished plan.
> But now a not-preprocessed copy is managing to get through to
> the executor, which promptly spits up because it doesn't know
> what to do with a SubLink.

> We could probably hack this in a localized way by ensuring that
> we push a preprocessed copy of the PHV into the left plan's tlist.
> (One way to get that would be to look in the placeholder_list for the
> correct phid.)  However, now that I've seen this I have very little
> faith that there aren't other bugs of the same ilk, that somehow
> we've not seen up to now.  I'm thinking about what we must do to
> ensure that all PHVs are preprocessed once and only once, perhaps
> at the moment they get put into the placeholder_list.

I've traced through this in more detail, and found that it's
inaccurate to claim that the PHV's expression escaped preprocessing
altogether.  (If it had, there would be more failure modes besides
"unprocessed SubLink", and we'd likely have noticed sooner.)
Rather, the problem is that SS_process_sublinks() skips replacement of
SubLinks within PlaceHolderVars that have phlevelsup > 0, expecting
that to be handled later.  So the sequence of events that Alexander's
test case causes is:

1. We pull up the first sub-select "(SELECT (SELECT true) AS c FROM t,
LATERAL (SELECT b LIMIT 1))", causing the "WHERE c" in the later
LATERAL sub-select to be replaced by PHV((SELECT true)); a PHV is
needed since the pulled-up sub-select was under an outer join.
The PHV has phlevelsup = 1 since it's pushed down into a sub-select.

2. Expression pre-processing happens in the separately-planned
LATERAL sub-select, and while it does most of what's needed, it
explicitly skips replacement of the PHV's SubLink.

3. SS_replace_correlation_vars pulls the PHV out of the LATERAL
sub-select, replacing it with a Param and adding it to the
plan_params of the outer query level.  Now it has phlevelsup = 0,
but there's still a SubLink inside it.

4. The code added by a16ef313f2 copies the PHV verbatim from
the plan_params list (via a subquery rel's subplan_params and
root->curOuterParams) into the outer subplan's tlist.  Kaboom!

Now, if the PHV matched something that was already in the
outer subplan's tlist (recall it only needs to match by phid)
then we're safe, because that copy would have gotten fixed up
during extract_lateral_references.  But in this example that
doesn't happen, probably because the PHV contains no Vars
so it doesn't get put into any baserel's reltarget.

I'm still trying to wrap my head around whether there are any
related failure modes in released branches.  Steps 1-3 certainly
have been happening just like that for a long time, so that there
can be a PHV with an unexpanded SubLink floating around the
outer query level's data structures.  Is the a16ef313f2 code
really the only way that that version of the PHV can make its way
into the emitted plan tree?  Maybe, but I'm far from convinced.

Anyway, with one eye on the possibility that we'll need to back-patch
this in some form, I went for the localized fix of ensuring that
we use a copy of the PHV that's been through the normal processing.
I do want to look into restructuring the handling of these PHVs
to make this less messy, but that's not a job to undertake for v18
(much less if we have to back-patch).

One thing worth noting is that there's a visible shortcoming in the
generated plans for the new test cases: there are two copies of the
InitPlan that arises from the PHV's SubLink.  One of them is
unreferenced and hence doesn't get used at runtime.  That happens
because we are performing SS_process_sublinks on the PHV twice.
It's not fatal, but it's not great either.  Perhaps a better design
could avoid that.  I definitely don't want to adopt a fix that
results in still another InitPlan, so adding YA invocation of
preprocessing doesn't sound good.

            regards, tom lane

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 8baf36ba4b7..8011c3c4bc8 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4344,6 +4344,7 @@ create_nestloop_plan(PlannerInfo *root,
     NestLoop   *join_plan;
     Plan       *outer_plan;
     Plan       *inner_plan;
+    Relids        outerrelids;
     List       *tlist = build_path_tlist(root, &best_path->jpath.path);
     List       *joinrestrictclauses = best_path->jpath.joinrestrictinfo;
     List       *joinclauses;
@@ -4374,8 +4375,8 @@ create_nestloop_plan(PlannerInfo *root,
     outer_plan = create_plan_recurse(root, best_path->jpath.outerjoinpath, 0);

     /* For a nestloop, include outer relids in curOuterRels for inner side */
-    root->curOuterRels = bms_union(root->curOuterRels,
-                                   best_path->jpath.outerjoinpath->parent->relids);
+    outerrelids = best_path->jpath.outerjoinpath->parent->relids;
+    root->curOuterRels = bms_union(root->curOuterRels, outerrelids);

     inner_plan = create_plan_recurse(root, best_path->jpath.innerjoinpath, 0);

@@ -4415,7 +4416,8 @@ create_nestloop_plan(PlannerInfo *root,
      * node, and remove them from root->curOuterParams.
      */
     nestParams = identify_current_nestloop_params(root,
-                                                  best_path->jpath.outerjoinpath);
+                                                  outerrelids,
+                                                  PATH_REQ_OUTER((Path *) best_path));

     /*
      * While nestloop parameters that are Vars had better be available from
@@ -4423,32 +4425,77 @@ create_nestloop_plan(PlannerInfo *root,
      * that are PHVs won't be.  In such cases we must add them to the
      * outer_plan's tlist, since the executor's NestLoopParam machinery
      * requires the params to be simple outer-Var references to that tlist.
+     * (This is cheating a little bit, because the outer path's required-outer
+     * relids might not be enough to allow evaluating such a PHV.  But in
+     * practice, if we could have evaluated the PHV at the nestloop node, we
+     * can do so in the outer plan too.)
      */
     outer_tlist = outer_plan->targetlist;
     outer_parallel_safe = outer_plan->parallel_safe;
     foreach(lc, nestParams)
     {
         NestLoopParam *nlp = (NestLoopParam *) lfirst(lc);
+        PlaceHolderVar *phv;
         TargetEntry *tle;

         if (IsA(nlp->paramval, Var))
             continue;            /* nothing to do for simple Vars */
-        if (tlist_member((Expr *) nlp->paramval, outer_tlist))
+        /* Otherwise it must be a PHV */
+        phv = castNode(PlaceHolderVar, nlp->paramval);
+
+        if (tlist_member((Expr *) phv, outer_tlist))
             continue;            /* already available */

+        /*
+         * Deal with an even edgier edge case: if the PHV was pulled up out of
+         * a subquery and it contains a subquery that was originally pushed
+         * down from this query level, then that will still be represented as
+         * a SubLink, because SS_process_sublinks won't recurse into outer
+         * PHVs, so it didn't get transformed during expression preprocessing
+         * in the subquery.  We need a version of the PHV that has a SubPlan,
+         * which we can get from the current query level's placeholder_list.
+         * This is quite grotty of course, but dealing with it earlier in the
+         * handling of subplan params would be just as grotty, and it might
+         * end up being a waste of cycles if we don't decide to treat the PHV
+         * as a NestLoopParam.  (Perhaps that whole mechanism should be
+         * redesigned someday, but today is not that day.)
+         *
+         * It's not important to change the NestLoopParam tree, because the
+         * two PHVs will still be considered equal() and thus setrefs.c will
+         * still be able to replace the NestLoopParam's PHV with an outer-plan
+         * reference Var.
+         *
+         * In either case, make sure we have a copy of the PHV to put into
+         * outer_tlist, just for safety.
+         */
+        if (root->parse->hasSubLinks)
+        {
+            PlaceHolderInfo *phinfo = find_placeholder_info(root, phv);
+            PlaceHolderVar *newphv = copyObject(phinfo->ph_var);
+
+            /*
+             * The ph_var will have empty nullingrels, but we need the right
+             * nullingrels in the PHV we push into outer_tlist.  Other fields
+             * should match already.
+             */
+            newphv->phnullingrels = phv->phnullingrels;
+            phv = newphv;
+        }
+        else
+            phv = copyObject(phv);
+
         /* Make a shallow copy of outer_tlist, if we didn't already */
         if (outer_tlist == outer_plan->targetlist)
             outer_tlist = list_copy(outer_tlist);
         /* ... and add the needed expression */
-        tle = makeTargetEntry((Expr *) copyObject(nlp->paramval),
+        tle = makeTargetEntry((Expr *) phv,
                               list_length(outer_tlist) + 1,
                               NULL,
                               true);
         outer_tlist = lappend(outer_tlist, tle);
         /* ... and track whether tlist is (still) parallel-safe */
         if (outer_parallel_safe)
-            outer_parallel_safe = is_parallel_safe(root,
-                                                   (Node *) nlp->paramval);
+            outer_parallel_safe = is_parallel_safe(root, (Node *) phv);
     }
     if (outer_tlist != outer_plan->targetlist)
         outer_plan = change_plan_targetlist(outer_plan, outer_tlist,
diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c
index 9836abf9479..5403ae89eab 100644
--- a/src/backend/optimizer/util/paramassign.c
+++ b/src/backend/optimizer/util/paramassign.c
@@ -599,9 +599,10 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
 }

 /*
- * Identify any NestLoopParams that should be supplied by a NestLoop plan
- * node with the specified lefthand input path.  Remove them from the active
- * root->curOuterParams list and return them as the result list.
+ * Identify any NestLoopParams that should be supplied by a NestLoop
+ * plan node with the specified lefthand rels and required-outer rels.
+ * Remove them from the active root->curOuterParams list and return
+ * them as the result list.
  *
  * XXX Here we also hack up the returned Vars and PHVs so that they do not
  * contain nullingrel sets exceeding what is available from the outer side.
@@ -626,11 +627,11 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
  * subquery, which'd be unduly expensive.
  */
 List *
-identify_current_nestloop_params(PlannerInfo *root, Path *leftpath)
+identify_current_nestloop_params(PlannerInfo *root,
+                                 Relids leftrelids,
+                                 Relids outerrelids)
 {
     List       *result;
-    Relids        leftrelids = leftpath->parent->relids;
-    Relids        outerrelids = PATH_REQ_OUTER(leftpath);
     Relids        allleftrelids;
     ListCell   *cell;

diff --git a/src/include/optimizer/paramassign.h b/src/include/optimizer/paramassign.h
index d30d20de299..bbf7214289b 100644
--- a/src/include/optimizer/paramassign.h
+++ b/src/include/optimizer/paramassign.h
@@ -30,7 +30,8 @@ extern Param *replace_nestloop_param_placeholdervar(PlannerInfo *root,
 extern void process_subquery_nestloop_params(PlannerInfo *root,
                                              List *subplan_params);
 extern List *identify_current_nestloop_params(PlannerInfo *root,
-                                              Path *leftpath);
+                                              Relids leftrelids,
+                                              Relids outerrelids);
 extern Param *generate_new_exec_param(PlannerInfo *root, Oid paramtype,
                                       int32 paramtypmod, Oid paramcollation);
 extern int    assign_special_exec_param(PlannerInfo *root);
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index c292f04fdba..6266820b69a 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -4119,6 +4119,91 @@ select * from int8_tbl t1
 (16 rows)

 rollback;
+-- PHVs containing SubLinks are quite tricky to get right
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+  inner join
+    (select (select true) as x
+       from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1
+       where i4.f1 = 0) ss2 on true
+  right join (select false as z) ss3 on true,
+  lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+                           QUERY PLAN
+----------------------------------------------------------------
+ Nested Loop
+   Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2)
+   InitPlan 1
+     ->  Result
+           Output: true
+   InitPlan 2
+     ->  Result
+           Output: true
+   ->  Seq Scan on public.int4_tbl i4
+         Output: i4.f1
+         Filter: (i4.f1 = 0)
+   ->  Nested Loop
+         Output: i8.q1, i8.q2, (i8.q2)
+         ->  Subquery Scan on ss1
+               Output: ss1.y, (InitPlan 1).col1
+               ->  Limit
+                     Output: NULL::integer
+                     ->  Result
+                           Output: NULL::integer
+         ->  Nested Loop
+               Output: i8.q1, i8.q2, (i8.q2)
+               ->  Seq Scan on public.int8_tbl i8
+                     Output: i8.q1, i8.q2
+                     Filter: (i8.q2 = 123)
+               ->  Limit
+                     Output: (i8.q2)
+                     ->  Result
+                           Output: i8.q2
+                           One-Time Filter: ((InitPlan 1).col1)
+(29 rows)
+
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+  inner join
+    (select (select true) as x
+       from int4_tbl i4, lateral (select 1 as y limit 1) ss1
+       where i4.f1 = 0) ss2 on true
+  right join (select false as z) ss3 on true,
+  lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+                           QUERY PLAN
+----------------------------------------------------------------
+ Nested Loop
+   Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2)
+   InitPlan 1
+     ->  Result
+           Output: true
+   InitPlan 2
+     ->  Result
+           Output: true
+   ->  Limit
+         Output: NULL::integer
+         ->  Result
+               Output: NULL::integer
+   ->  Nested Loop
+         Output: i8.q1, i8.q2, (i8.q2)
+         ->  Seq Scan on public.int4_tbl i4
+               Output: i4.f1, (InitPlan 1).col1
+               Filter: (i4.f1 = 0)
+         ->  Nested Loop
+               Output: i8.q1, i8.q2, (i8.q2)
+               ->  Seq Scan on public.int8_tbl i8
+                     Output: i8.q1, i8.q2
+                     Filter: (i8.q2 = 123)
+               ->  Limit
+                     Output: (i8.q2)
+                     ->  Result
+                           Output: i8.q2
+                           One-Time Filter: ((InitPlan 1).col1)
+(27 rows)
+
 -- Test proper handling of appendrel PHVs during useless-RTE removal
 explain (costs off)
 select * from
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 88d2204e447..6b7a26cf295 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1361,6 +1361,29 @@ select * from int8_tbl t1
   on true;
 rollback;

+-- PHVs containing SubLinks are quite tricky to get right
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+  inner join
+    (select (select true) as x
+       from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1
+       where i4.f1 = 0) ss2 on true
+  right join (select false as z) ss3 on true,
+  lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+  inner join
+    (select (select true) as x
+       from int4_tbl i4, lateral (select 1 as y limit 1) ss1
+       where i4.f1 = 0) ss2 on true
+  right join (select false as z) ss3 on true,
+  lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+
 -- Test proper handling of appendrel PHVs during useless-RTE removal
 explain (costs off)
 select * from