Fix "wrong varnullingrels" for Memoize's lateral references, too.

The issue fixed in commit bfd332b3f can also bite Memoize plans,
because of the separate copies of lateral reference Vars made
by paraminfo_get_equal_hashops.  Apply the same hacky fix there.

(In passing, clean up shaky grammar in the existing comments
for this function.)

Richard Guo

Discussion: https://postgr.es/m/CAMbWs4-krwk0Wbd6WdufMAupuou_Ua73ijQ4XQCr1Mb5BaVtKQ@mail.gmail.com

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index cd80e61fd75d9480d806adc7afbc0433c2d39e24..5ba266fdb6cda4d30b578c05c13eba1ffceb66d3 100644 (file)
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -421,12 +421,33 @@ have_unsafe_outer_join_ref(PlannerInfo *root,
 
 /*
  * paraminfo_get_equal_hashops
- *     Determine if param_info and innerrel's lateral_vars can be hashed.
- *     Returns true the hashing is possible, otherwise return false.
+ *     Determine if the clauses in param_info and innerrel's lateral_vars
+ *     can be hashed.
+ *     Returns true if hashing is possible, otherwise false.
  *
- * Additionally we also collect the outer exprs and the hash operators for
- * each parameter to innerrel.  These set in 'param_exprs', 'operators' and
- * 'binary_mode' when we return true.
+ * Additionally, on success we collect the outer expressions and the
+ * appropriate equality operators for each hashable parameter to innerrel.
+ * These are returned in parallel lists in *param_exprs and *operators.
+ * We also set *binary_mode to indicate whether strict binary matching is
+ * required.
+ *
+ * A complication is that innerrel's lateral_vars may contain nullingrel
+ * markers that need adjustment.  This occurs if we have applied outer join
+ * identity 3,
+ *     (A leftjoin B on (Pab)) leftjoin C on (Pb*c)
+ *     = A leftjoin (B leftjoin C on (Pbc)) on (Pab)
+ * and C contains lateral references to B.  It's still safe to apply the
+ * identity, but the parser will have created those references in the form
+ * "b*" (i.e., with varnullingrels listing the A/B join), while what we will
+ * have available from the nestloop's outer side is just "b".  We deal with
+ * that here by stripping the nullingrels down to what is available from the
+ * outer side according to outerrel->relids.
+ * That fixes matters for the case of forward application of identity 3.
+ * If the identity was applied in the reverse direction, we will have
+ * innerrel's lateral_vars containing too few nullingrel bits rather than
+ * too many.  Currently, that causes no problems because setrefs.c applies
+ * only a subset check to nullingrels in NestLoopParams, but we'd have to
+ * work harder if we ever want to tighten that check.
  */
 static bool
 paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
@@ -441,6 +462,7 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
    *operators = NIL;
    *binary_mode = false;
 
+   /* Add join clauses from param_info to the hash key */
    if (param_info != NULL)
    {
        List       *clauses = param_info->ppi_clauses;
@@ -510,7 +532,7 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
        Node       *expr = (Node *) lfirst(lc);
        TypeCacheEntry *typentry;
 
-       /* Reject if there are any volatile functions */
+       /* Reject if there are any volatile functions in PHVs */
        if (contain_volatile_functions(expr))
        {
            list_free(*operators);
@@ -521,7 +543,7 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
        typentry = lookup_type_cache(exprType(expr),
                                     TYPECACHE_HASH_PROC | TYPECACHE_EQ_OPR);
 
-       /* can't use a memoize node without a valid hash equals operator */
+       /* can't use memoize without a valid hash proc and equals operator */
        if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr))
        {
            list_free(*operators);
@@ -529,6 +551,25 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
            return false;
        }
 
+       /* OK, but adjust its nullingrels before adding it to result */
+       expr = copyObject(expr);
+       if (IsA(expr, Var))
+       {
+           Var        *var = (Var *) expr;
+
+           var->varnullingrels = bms_intersect(var->varnullingrels,
+                                               outerrel->relids);
+       }
+       else if (IsA(expr, PlaceHolderVar))
+       {
+           PlaceHolderVar *phv = (PlaceHolderVar *) expr;
+
+           phv->phnullingrels = bms_intersect(phv->phnullingrels,
+                                              outerrel->relids);
+       }
+       else
+           Assert(false);
+
        *operators = lappend_oid(*operators, typentry->eq_opr);
        *param_exprs = lappend(*param_exprs, expr);
 

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 3585a703fbd66703ea112c6c85bbcf49d8110b0e..ec5552327fbd90908f694c544596a46cbb84dd15 100644 (file)
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -2289,11 +2289,11 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset)
             * the outer-join level at which they are used, Vars seen in the
             * NestLoopParam expression may have nullingrels that are just a
             * subset of those in the Vars actually available from the outer
-            * side.  Another case that can cause that to happen is explained
-            * in the comments for process_subquery_nestloop_params.  Not
-            * checking this exactly is a bit grotty, but the work needed to
-            * make things match up perfectly seems well out of proportion to
-            * the value.
+            * side.  Lateral references can create the same situation, as
+            * explained in the comments for process_subquery_nestloop_params
+            * and paraminfo_get_equal_hashops.  Not checking this exactly is
+            * a bit grotty, but the work needed to make things match up
+            * perfectly seems well out of proportion to the value.
             */
            nlp->paramval = (Var *) fix_upper_expr(root,
                                                   (Node *) nlp->paramval,

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 4999c99f3bcde046e38bf2fda4433063e98f1cbd..98b2667821e78e9f9da8c42aaa1813926b8e64b3 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -2607,6 +2607,27 @@ select * from int8_tbl t1
                      Filter: (q1 = t2.q1)
 (8 rows)
 
+explain (costs off)
+select * from onek t1
+    left join onek t2 on true
+    left join lateral
+      (select * from onek t3 where t3.two = t2.two offset 0) s
+      on t2.unique1 = 1;
+                    QUERY PLAN                    
+--------------------------------------------------
+ Nested Loop Left Join
+   ->  Seq Scan on onek t1
+   ->  Materialize
+         ->  Nested Loop Left Join
+               Join Filter: (t2.unique1 = 1)
+               ->  Seq Scan on onek t2
+               ->  Memoize
+                     Cache Key: t2.two
+                     Cache Mode: binary
+                     ->  Seq Scan on onek t3
+                           Filter: (two = t2.two)
+(11 rows)
+
 --
 -- check a case where we formerly got confused by conflicting sort orders
 -- in redundant merge join path keys

diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 56ca759772b1b933e8b99c908b1afe13387d313a..7daa390b1d46f5632b0175e108de0d188979cd8e 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -521,6 +521,13 @@ select * from int8_tbl t1
       (select * from int8_tbl t3 where t3.q1 = t2.q1 offset 0) s
       on t2.q1 = 1;
 
+explain (costs off)
+select * from onek t1
+    left join onek t2 on true
+    left join lateral
+      (select * from onek t3 where t3.two = t2.two offset 0) s
+      on t2.unique1 = 1;
+
 --
 -- check a case where we formerly got confused by conflicting sort orders
 -- in redundant merge join path keys