]> granicus.if.org Git - postgresql/commitdiff
Fix planner's handling of outer PlaceHolderVars within subqueries.
authorTom Lane <tgl@sss.pgh.pa.us>
Sat, 24 Mar 2012 20:21:39 +0000 (16:21 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Sat, 24 Mar 2012 20:21:39 +0000 (16:21 -0400)
For some reason, in the original coding of the PlaceHolderVar mechanism
I had supposed that PlaceHolderVars couldn't propagate into subqueries.
That is of course entirely possible.  When it happens, we need to treat
an outer-level PlaceHolderVar much like an outer Var or Aggref, that is
SS_replace_correlation_vars() needs to replace the PlaceHolderVar with
a Param, and then when building the finished SubPlan we have to provide
the PlaceHolderVar expression as an actual parameter for the SubPlan.
The handling of the contained expression is a bit delicate but it can be
treated exactly like an Aggref's expression.

In addition to the missing logic in subselect.c, prepjointree.c was failing
to search subqueries for PlaceHolderVars that need their relids adjusted
during subquery pullup.  It looks like everyplace else that touches
PlaceHolderVars got it right, though.

Per report from Mark Murawski.  In 9.1 and HEAD, queries affected by this
oversight would fail with "ERROR: Upper-level PlaceHolderVar found where
not expected".  But in 9.0 and 8.4, you'd silently get possibly-wrong
answers, since the value transmitted into the subquery wouldn't go to null
when it should.

src/backend/optimizer/plan/subselect.c
src/backend/optimizer/prep/prepjointree.c
src/include/nodes/relation.h
src/test/regress/expected/join.out
src/test/regress/sql/join.sql

index f30f02f266d2596942d89b3e3c7b8d4d6ff7fe2c..fb6c7045484e9ed68c10f44bb0adaaecd6c599d4 100644 (file)
@@ -191,25 +191,22 @@ assign_nestloop_param_var(PlannerInfo *root, Var *var)
 }
 
 /*
- * Generate a Param node to replace the given PlaceHolderVar, which will be
- * supplied from an upper NestLoop join node.
+ * Select a PARAM_EXEC number to identify the given PlaceHolderVar.
+ * If the PlaceHolderVar already has a param slot, return that one.
  *
- * This is just like assign_nestloop_param_var, except for PlaceHolderVars.
+ * This is just like assign_param_for_var, except for PlaceHolderVars.
  */
-Param *
-assign_nestloop_param_placeholdervar(PlannerInfo *root, PlaceHolderVar *phv)
+static int
+assign_param_for_placeholdervar(PlannerInfo *root, PlaceHolderVar *phv)
 {
-       Param      *retval;
        ListCell   *ppl;
        PlannerParamItem *pitem;
        Index           abslevel;
        int                     i;
 
-       Assert(phv->phlevelsup == 0);
-       abslevel = root->query_level;
+       abslevel = root->query_level - phv->phlevelsup;
 
        /* If there's already a paramlist entry for this same PHV, just use it */
-       /* We assume comparing the PHIDs is sufficient */
        i = 0;
        foreach(ppl, root->glob->paramlist)
        {
@@ -218,25 +215,77 @@ assign_nestloop_param_placeholdervar(PlannerInfo *root, PlaceHolderVar *phv)
                {
                        PlaceHolderVar *pphv = (PlaceHolderVar *) pitem->item;
 
+                       /* We assume comparing the PHIDs is sufficient */
                        if (pphv->phid == phv->phid)
-                               break;
+                               return i;
                }
                i++;
        }
 
-       if (ppl == NULL)
+       /* Nope, so make a new one */
+       phv = (PlaceHolderVar *) copyObject(phv);
+       if (phv->phlevelsup != 0)
        {
-               /* Nope, so make a new one */
-               phv = (PlaceHolderVar *) copyObject(phv);
+               IncrementVarSublevelsUp((Node *) phv, -((int) phv->phlevelsup), 0);
+               Assert(phv->phlevelsup == 0);
+       }
 
-               pitem = makeNode(PlannerParamItem);
-               pitem->item = (Node *) phv;
-               pitem->abslevel = abslevel;
+       pitem = makeNode(PlannerParamItem);
+       pitem->item = (Node *) phv;
+       pitem->abslevel = abslevel;
 
-               root->glob->paramlist = lappend(root->glob->paramlist, pitem);
+       root->glob->paramlist = lappend(root->glob->paramlist, pitem);
 
-               /* i is already the correct list index for the new item */
-       }
+       /* i is already the correct list index for the new item */
+       return i;
+}
+
+/*
+ * Generate a Param node to replace the given PlaceHolderVar,
+ * which is expected to have phlevelsup > 0 (ie, it is not local).
+ *
+ * This is just like replace_outer_var, except for PlaceHolderVars.
+ */
+static Param *
+replace_outer_placeholdervar(PlannerInfo *root, PlaceHolderVar *phv)
+{
+       Param      *retval;
+       int                     i;
+
+       Assert(phv->phlevelsup > 0 && phv->phlevelsup < root->query_level);
+
+       /*
+        * Find the PlaceHolderVar in root->glob->paramlist, or add it if not
+        * present.
+        */
+       i = assign_param_for_placeholdervar(root, phv);
+
+       retval = makeNode(Param);
+       retval->paramkind = PARAM_EXEC;
+       retval->paramid = i;
+       retval->paramtype = exprType((Node *) phv->phexpr);
+       retval->paramtypmod = exprTypmod((Node *) phv->phexpr);
+       retval->paramcollid = exprCollation((Node *) phv->phexpr);
+       retval->location = -1;
+
+       return retval;
+}
+
+/*
+ * Generate a Param node to replace the given PlaceHolderVar, which will be
+ * supplied from an upper NestLoop join node.
+ *
+ * This is just like assign_nestloop_param_var, except for PlaceHolderVars.
+ */
+Param *
+assign_nestloop_param_placeholdervar(PlannerInfo *root, PlaceHolderVar *phv)
+{
+       Param      *retval;
+       int                     i;
+
+       Assert(phv->phlevelsup == 0);
+
+       i = assign_param_for_placeholdervar(root, phv);
 
        retval = makeNode(Param);
        retval->paramkind = PARAM_EXEC;
@@ -555,17 +604,19 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
                        Node       *arg;
 
                        /*
-                        * The Var or Aggref has already been adjusted to have the correct
-                        * varlevelsup or agglevelsup.  We probably don't even need to
-                        * copy it again, but be safe.
+                        * The Var, PlaceHolderVar, or Aggref has already been adjusted to
+                        * have the correct varlevelsup, phlevelsup, or agglevelsup.  We
+                        * probably don't even need to copy it again, but be safe.
                         */
                        arg = copyObject(pitem->item);
 
                        /*
-                        * If it's an Aggref, its arguments might contain SubLinks, which
-                        * have not yet been processed.  Do that now.
+                        * If it's a PlaceHolderVar or Aggref, its arguments might contain
+                        * SubLinks, which have not yet been processed (see the comments
+                        * for SS_replace_correlation_vars).  Do that now.
                         */
-                       if (IsA(arg, Aggref))
+                       if (IsA(arg, PlaceHolderVar) ||
+                               IsA(arg, Aggref))
                                arg = SS_process_sublinks(root, arg, false);
 
                        splan->parParam = lappend_int(splan->parParam, paramid);
@@ -1668,24 +1719,25 @@ convert_EXISTS_to_ANY(PlannerInfo *root, Query *subselect,
 /*
  * Replace correlation vars (uplevel vars) with Params.
  *
- * Uplevel aggregates are replaced, too.
+ * Uplevel PlaceHolderVars and aggregates are replaced, too.
  *
  * Note: it is critical that this runs immediately after SS_process_sublinks.
- * Since we do not recurse into the arguments of uplevel aggregates, they will
- * get copied to the appropriate subplan args list in the parent query with
- * uplevel vars not replaced by Params, but only adjusted in level (see
- * replace_outer_agg). That's exactly what we want for the vars of the parent
- * level --- but if an aggregate's argument contains any further-up variables,
- * they have to be replaced with Params in their turn. That will happen when
- * the parent level runs SS_replace_correlation_vars.  Therefore it must do
- * so after expanding its sublinks to subplans.  And we don't want any steps
- * in between, else those steps would never get applied to the aggregate
- * argument expressions, either in the parent or the child level.
+ * Since we do not recurse into the arguments of uplevel PHVs and aggregates,
+ * they will get copied to the appropriate subplan args list in the parent
+ * query with uplevel vars not replaced by Params, but only adjusted in level
+ * (see replace_outer_placeholdervar and replace_outer_agg).  That's exactly
+ * what we want for the vars of the parent level --- but if a PHV's or
+ * aggregate's argument contains any further-up variables, they have to be
+ * replaced with Params in their turn. That will happen when the parent level
+ * runs SS_replace_correlation_vars.  Therefore it must do so after expanding
+ * its sublinks to subplans.  And we don't want any steps in between, else
+ * those steps would never get applied to the argument expressions, either in
+ * the parent or the child level.
  *
  * Another fairly tricky thing going on here is the handling of SubLinks in
- * the arguments of uplevel aggregates.  Those are not touched inside the
- * intermediate query level, either.  Instead, SS_process_sublinks recurses
- * on them after copying the Aggref expression into the parent plan level
+ * the arguments of uplevel PHVs/aggregates.  Those are not touched inside the
+ * intermediate query level, either.  Instead, SS_process_sublinks recurses on
+ * them after copying the PHV or Aggref expression into the parent plan level
  * (this is actually taken care of in build_subplan).
  */
 Node *
@@ -1705,6 +1757,12 @@ replace_correlation_vars_mutator(Node *node, PlannerInfo *root)
                if (((Var *) node)->varlevelsup > 0)
                        return (Node *) replace_outer_var(root, (Var *) node);
        }
+       if (IsA(node, PlaceHolderVar))
+       {
+               if (((PlaceHolderVar *) node)->phlevelsup > 0)
+                       return (Node *) replace_outer_placeholdervar(root,
+                                                                                                       (PlaceHolderVar *) node);
+       }
        if (IsA(node, Aggref))
        {
                if (((Aggref *) node)->agglevelsup > 0)
@@ -1764,12 +1822,17 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
        }
 
        /*
-        * Don't recurse into the arguments of an outer aggregate here. Any
-        * SubLinks in the arguments have to be dealt with at the outer query
-        * level; they'll be handled when build_subplan collects the Aggref into
-        * the arguments to be passed down to the current subplan.
+        * Don't recurse into the arguments of an outer PHV or aggregate here.
+        * Any SubLinks in the arguments have to be dealt with at the outer query
+        * level; they'll be handled when build_subplan collects the PHV or Aggref
+        * into the arguments to be passed down to the current subplan.
         */
-       if (IsA(node, Aggref))
+       if (IsA(node, PlaceHolderVar))
+       {
+               if (((PlaceHolderVar *) node)->phlevelsup > 0)
+                       return node;
+       }
+       else if (IsA(node, Aggref))
        {
                if (((Aggref *) node)->agglevelsup > 0)
                        return node;
index c1b2f1db11adc7785db84b6e9da8cebae753ee3a..47ddae6992f7cf33431afe55aacd0a6a4e449f95 100644 (file)
@@ -2069,8 +2069,6 @@ reduce_outer_joins_pass2(Node *jtnode,
  *
  * Find any PlaceHolderVar nodes in the given tree that reference the
  * pulled-up relid, and change them to reference the replacement relid(s).
- * We do not need to recurse into subqueries, since no subquery of the current
- * top query could (yet) contain such a reference.
  *
  * NOTE: although this has the form of a walker, we cheat and modify the
  * nodes in-place.     This should be OK since the tree was copied by
@@ -2081,6 +2079,7 @@ reduce_outer_joins_pass2(Node *jtnode,
 typedef struct
 {
        int                     varno;
+       int                     sublevels_up;
        Relids          subrelids;
 } substitute_multiple_relids_context;
 
@@ -2094,7 +2093,8 @@ substitute_multiple_relids_walker(Node *node,
        {
                PlaceHolderVar *phv = (PlaceHolderVar *) node;
 
-               if (bms_is_member(context->varno, phv->phrels))
+               if (phv->phlevelsup == context->sublevels_up &&
+                       bms_is_member(context->varno, phv->phrels))
                {
                        phv->phrels = bms_union(phv->phrels,
                                                                        context->subrelids);
@@ -2103,6 +2103,18 @@ substitute_multiple_relids_walker(Node *node,
                }
                /* fall through to examine children */
        }
+       if (IsA(node, Query))
+       {
+               /* Recurse into subselects */
+               bool            result;
+
+               context->sublevels_up++;
+               result = query_tree_walker((Query *) node,
+                                                                  substitute_multiple_relids_walker,
+                                                                  (void *) context, 0);
+               context->sublevels_up--;
+               return result;
+       }
        /* Shouldn't need to handle planner auxiliary nodes here */
        Assert(!IsA(node, SpecialJoinInfo));
        Assert(!IsA(node, AppendRelInfo));
@@ -2119,6 +2131,7 @@ substitute_multiple_relids(Node *node, int varno, Relids subrelids)
        substitute_multiple_relids_context context;
 
        context.varno = varno;
+       context.sublevels_up = 0;
        context.subrelids = subrelids;
 
        /*
index d901a85e7ac680d89d8e71569da73d901991d771..6606e67055b160750241bf57f18929442b76fdca 100644 (file)
@@ -1445,8 +1445,10 @@ typedef struct MinMaxAggInfo
  * from a NestLoop node of that level to its inner scan.  The varlevelsup
  * value in the Var will always be zero.
  *
- * A PlaceHolderVar: this works much like the Var case.  It is currently
- * only needed for NestLoop parameters, not outer references.
+ * A PlaceHolderVar: this works much like the Var case, except that the
+ * entry is a PlaceHolderVar node with a contained expression.  The PHV
+ * will have phlevelsup = 0, and the contained expression is adjusted
+ * to match in level.
  *
  * An Aggref (with an expression tree representing its argument): the slot
  * represents an aggregate expression that is an outer reference for some
index 5447a26f447a1edfca5d42c276145ca02da130c5..c0c72833339c65af560a55781758b4f5ac0f2079 100644 (file)
@@ -2571,6 +2571,53 @@ SELECT qq, unique1
  456 |    7318
 (3 rows)
 
+--
+-- test case where a PlaceHolderVar is propagated into a subquery
+--
+explain (costs off)
+select * from
+  int8_tbl t1 left join
+  (select q1 as x, 42 as y from int8_tbl t2) ss
+  on t1.q2 = ss.x
+where
+  1 = (select 1 from int8_tbl t3 where ss.y is not null limit 1)
+order by 1,2;
+                        QUERY PLAN                         
+-----------------------------------------------------------
+ Sort
+   Sort Key: t1.q1, t1.q2
+   ->  Hash Left Join
+         Hash Cond: (t1.q2 = t2.q1)
+         Filter: (1 = (SubPlan 1))
+         ->  Seq Scan on int8_tbl t1
+         ->  Hash
+               ->  Seq Scan on int8_tbl t2
+         SubPlan 1
+           ->  Limit
+                 ->  Result
+                       One-Time Filter: ((42) IS NOT NULL)
+                       ->  Seq Scan on int8_tbl t3
+(13 rows)
+
+select * from
+  int8_tbl t1 left join
+  (select q1 as x, 42 as y from int8_tbl t2) ss
+  on t1.q2 = ss.x
+where
+  1 = (select 1 from int8_tbl t3 where ss.y is not null limit 1)
+order by 1,2;
+        q1        |        q2        |        x         | y  
+------------------+------------------+------------------+----
+              123 | 4567890123456789 | 4567890123456789 | 42
+              123 | 4567890123456789 | 4567890123456789 | 42
+              123 | 4567890123456789 | 4567890123456789 | 42
+ 4567890123456789 |              123 |              123 | 42
+ 4567890123456789 |              123 |              123 | 42
+ 4567890123456789 | 4567890123456789 | 4567890123456789 | 42
+ 4567890123456789 | 4567890123456789 | 4567890123456789 | 42
+ 4567890123456789 | 4567890123456789 | 4567890123456789 | 42
+(8 rows)
+
 --
 -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE
 --
index 247fae11f9908809faad4145128781b4ac4d9ca9..2d53cf1725bd02c131ff3eb45f4b5eba00f64a35 100644 (file)
@@ -662,6 +662,27 @@ SELECT qq, unique1
   USING (qq)
   INNER JOIN tenk1 c ON qq = unique2;
 
+--
+-- test case where a PlaceHolderVar is propagated into a subquery
+--
+
+explain (costs off)
+select * from
+  int8_tbl t1 left join
+  (select q1 as x, 42 as y from int8_tbl t2) ss
+  on t1.q2 = ss.x
+where
+  1 = (select 1 from int8_tbl t3 where ss.y is not null limit 1)
+order by 1,2;
+
+select * from
+  int8_tbl t1 left join
+  (select q1 as x, 42 as y from int8_tbl t2) ss
+  on t1.q2 = ss.x
+where
+  1 = (select 1 from int8_tbl t3 where ss.y is not null limit 1)
+order by 1,2;
+
 --
 -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE
 --