Fix incorrect matching of subexpressions in outer-join plan nodes.

Previously we would re-use input subexpressions in all expression trees
attached to a Join plan node.  However, if it's an outer join and the
subexpression appears in the nullable-side input, this is potentially
incorrect for apparently-matching subexpressions that came from above
the outer join (ie, targetlist and qpqual expressions), because the
executor will treat the subexpression value as NULL when maybe it should
not be.

The case is fairly hard to hit because (a) you need a non-strict
subexpression (else NULL is correct), and (b) we don't usually compute
expressions in the outputs of non-toplevel plan nodes.  But we might do
so if the expressions are sort keys for a mergejoin, for example.

Probably in the long run we should make a more explicit distinction between
Vars appearing above and below an outer join, but that will be a major
planner redesign and not at all back-patchable.  For the moment, just hack
set_join_references so that it will not match any non-Var expressions
coming from nullable inputs to expressions that came from above the join.
(This is somewhat overkill, in that a strict expression could still be
matched, but it doesn't seem worth the effort to check that.)

Per report from Qingqing Zhou.  The added regression test case is based
on his example.

This has been broken for a very long time, so back-patch to all active
branches.

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 21512bd7de965bf64d42bfe8a5a7a507552c8a85..cc8d39f39bbee354e9e0127e5cb786f3851d7c2e 100644 (file)
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1231,19 +1231,13 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset)
    outer_itlist = build_tlist_index(outer_plan->targetlist);
    inner_itlist = build_tlist_index(inner_plan->targetlist);
 
-   /* All join plans have tlist, qual, and joinqual */
-   join->plan.targetlist = fix_join_expr(root,
-                                         join->plan.targetlist,
-                                         outer_itlist,
-                                         inner_itlist,
-                                         (Index) 0,
-                                         rtoffset);
-   join->plan.qual = fix_join_expr(root,
-                                   join->plan.qual,
-                                   outer_itlist,
-                                   inner_itlist,
-                                   (Index) 0,
-                                   rtoffset);
+   /*
+    * First process the joinquals (including merge or hash clauses).  These
+    * are logically below the join so they can always use all values
+    * available from the input tlists.  It's okay to also handle
+    * NestLoopParams now, because those couldn't refer to nullable
+    * subexpressions.
+    */
    join->joinqual = fix_join_expr(root,
                                   join->joinqual,
                                   outer_itlist,
@@ -1295,6 +1289,49 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset)
                                        rtoffset);
    }
 
+   /*
+    * Now we need to fix up the targetlist and qpqual, which are logically
+    * above the join.  This means they should not re-use any input expression
+    * that was computed in the nullable side of an outer join.  Vars and
+    * PlaceHolderVars are fine, so we can implement this restriction just by
+    * clearing has_non_vars in the indexed_tlist structs.
+    *
+    * XXX This is a grotty workaround for the fact that we don't clearly
+    * distinguish between a Var appearing below an outer join and the "same"
+    * Var appearing above it.  If we did, we'd not need to hack the matching
+    * rules this way.
+    */
+   switch (join->jointype)
+   {
+       case JOIN_LEFT:
+       case JOIN_SEMI:
+       case JOIN_ANTI:
+           inner_itlist->has_non_vars = false;
+           break;
+       case JOIN_RIGHT:
+           outer_itlist->has_non_vars = false;
+           break;
+       case JOIN_FULL:
+           outer_itlist->has_non_vars = false;
+           inner_itlist->has_non_vars = false;
+           break;
+       default:
+           break;
+   }
+
+   join->plan.targetlist = fix_join_expr(root,
+                                         join->plan.targetlist,
+                                         outer_itlist,
+                                         inner_itlist,
+                                         (Index) 0,
+                                         rtoffset);
+   join->plan.qual = fix_join_expr(root,
+                                   join->plan.qual,
+                                   outer_itlist,
+                                   inner_itlist,
+                                   (Index) 0,
+                                   rtoffset);
+
    pfree(outer_itlist);
    pfree(inner_itlist);
 }
@@ -1573,7 +1610,9 @@ search_indexed_tlist_for_var(Var *var, indexed_tlist *itlist,
  * If no match, return NULL.
  *
  * NOTE: it is a waste of time to call this unless itlist->has_ph_vars or
- * itlist->has_non_vars
+ * itlist->has_non_vars.  Furthermore, set_join_references() relies on being
+ * able to prevent matching of non-Vars by clearing itlist->has_non_vars,
+ * so there's a correctness reason not to call it unless that's set.
  */
 static Var *
 search_indexed_tlist_for_non_var(Node *node,

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 988428bbbe29f0a6c8b937577d2e32eae0f1e7ec..9d5e69c3083e378e295cf56fd7d3e57712f2be4b 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -3096,6 +3096,54 @@ explain (costs off)
          Index Cond: (unique2 = 42)
 (6 rows)
 
+--
+-- test that quals attached to an outer join have correct semantics,
+-- specifically that they don't re-use expressions computed below the join;
+-- we force a mergejoin so that coalesce(b.q1, 1) appears as a join input
+--
+set enable_hashjoin to off;
+set enable_nestloop to off;
+explain (verbose, costs off)
+  select a.q2, b.q1
+    from int8_tbl a left join int8_tbl b on a.q2 = coalesce(b.q1, 1)
+    where coalesce(b.q1, 1) > 0;
+                      QUERY PLAN                       
+-------------------------------------------------------
+ Merge Left Join
+   Output: a.q2, b.q1
+   Merge Cond: (a.q2 = (COALESCE(b.q1, 1::bigint)))
+   Filter: (COALESCE(b.q1, 1::bigint) > 0)
+   ->  Sort
+         Output: a.q2
+         Sort Key: a.q2
+         ->  Seq Scan on public.int8_tbl a
+               Output: a.q2
+   ->  Sort
+         Output: b.q1, (COALESCE(b.q1, 1::bigint))
+         Sort Key: (COALESCE(b.q1, 1::bigint))
+         ->  Seq Scan on public.int8_tbl b
+               Output: b.q1, COALESCE(b.q1, 1::bigint)
+(14 rows)
+
+select a.q2, b.q1
+  from int8_tbl a left join int8_tbl b on a.q2 = coalesce(b.q1, 1)
+  where coalesce(b.q1, 1) > 0;
+        q2         |        q1        
+-------------------+------------------
+ -4567890123456789 |                 
+               123 |              123
+               123 |              123
+               456 |                 
+  4567890123456789 | 4567890123456789
+  4567890123456789 | 4567890123456789
+  4567890123456789 | 4567890123456789
+  4567890123456789 | 4567890123456789
+  4567890123456789 | 4567890123456789
+  4567890123456789 | 4567890123456789
+(10 rows)
+
+reset enable_hashjoin;
+reset enable_nestloop;
 --
 -- test join removal
 --

diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 50dd37ec6722930ebfa6f53ae4e125d755ac7b06..ea69186f107dfc235e9934f0606aaa4a5b806785 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -907,6 +907,26 @@ explain (costs off)
 explain (costs off)
   select * from tenk1 a full join tenk1 b using(unique2) where unique2 = 42;
 
+--
+-- test that quals attached to an outer join have correct semantics,
+-- specifically that they don't re-use expressions computed below the join;
+-- we force a mergejoin so that coalesce(b.q1, 1) appears as a join input
+--
+
+set enable_hashjoin to off;
+set enable_nestloop to off;
+
+explain (verbose, costs off)
+  select a.q2, b.q1
+    from int8_tbl a left join int8_tbl b on a.q2 = coalesce(b.q1, 1)
+    where coalesce(b.q1, 1) > 0;
+select a.q2, b.q1
+  from int8_tbl a left join int8_tbl b on a.q2 = coalesce(b.q1, 1)
+  where coalesce(b.q1, 1) > 0;
+
+reset enable_hashjoin;
+reset enable_nestloop;
+
 --
 -- test join removal
 --