Fix miscomputation of direct_lateral_relids for join relations.

If a PlaceHolderVar is to be evaluated at a join relation, but
its value is only needed there and not at higher levels, we neglected
to update the joinrel's direct_lateral_relids to include the PHV's
source rel.  This causes problems because join_is_legal() then won't
allow joining the joinrel to the PHV's source rel at all, leading
to "failed to build any N-way joins" planner failures.

Per report from Andreas Seltenreich.  Back-patch to 9.5
where the problem originated.

Discussion: https://postgr.es/m/[email protected]

diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c
index 6abdc0299b5d16b3064ece5da99b140c132609c3..bf991018abb685a3ed6a3452f4c59602907c8fb5 100644 (file)
--- a/src/backend/optimizer/util/placeholder.c
+++ b/src/backend/optimizer/util/placeholder.c
@@ -404,8 +404,10 @@ add_placeholders_to_base_rels(PlannerInfo *root)
  *     and if they contain lateral references, add those references to the
  *     joinrel's direct_lateral_relids.
  *
- * A join rel should emit a PlaceHolderVar if (a) the PHV is needed above
- * this join level and (b) the PHV can be computed at or below this level.
+ * A join rel should emit a PlaceHolderVar if (a) the PHV can be computed
+ * at or below this join level and (b) the PHV is needed above this level.
+ * However, condition (a) is sufficient to add to direct_lateral_relids,
+ * as explained below.
  */
 void
 add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
@@ -418,11 +420,11 @@ add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
    {
        PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(lc);
 
-       /* Is it still needed above this joinrel? */
-       if (bms_nonempty_difference(phinfo->ph_needed, relids))
+       /* Is it computable here? */
+       if (bms_is_subset(phinfo->ph_eval_at, relids))
        {
-           /* Is it computable here? */
-           if (bms_is_subset(phinfo->ph_eval_at, relids))
+           /* Is it still needed above this joinrel? */
+           if (bms_nonempty_difference(phinfo->ph_needed, relids))
            {
                /* Yup, add it to the output */
                joinrel->reltarget->exprs = lappend(joinrel->reltarget->exprs,
@@ -450,12 +452,26 @@ add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
                    joinrel->reltarget->cost.startup += cost.startup;
                    joinrel->reltarget->cost.per_tuple += cost.per_tuple;
                }
-
-               /* Adjust joinrel's direct_lateral_relids as needed */
-               joinrel->direct_lateral_relids =
-                   bms_add_members(joinrel->direct_lateral_relids,
-                                   phinfo->ph_lateral);
            }
+
+           /*
+            * Also adjust joinrel's direct_lateral_relids to include the
+            * PHV's source rel(s).  We must do this even if we're not
+            * actually going to emit the PHV, otherwise join_is_legal() will
+            * reject valid join orderings.  (In principle maybe we could
+            * instead remove the joinrel's lateral_relids dependency; but
+            * that's complicated to get right, and cases where we're not
+            * going to emit the PHV are too rare to justify the work.)
+            *
+            * In principle we should only do this if the join doesn't yet
+            * include the PHV's source rel(s).  But our caller
+            * build_join_rel() will clean things up by removing the join's
+            * own relids from its direct_lateral_relids, so we needn't
+            * account for that here.
+            */
+           joinrel->direct_lateral_relids =
+               bms_add_members(joinrel->direct_lateral_relids,
+                               phinfo->ph_lateral);
        }
    }
 }

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 60b621b651f1b55560f89be6febae3bdad2bf274..a118041731989fe63654381fbc4d6d413fde4865 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -3010,6 +3010,70 @@ order by 1,2;
  4567890123456789 | 4567890123456789 | 4567890123456789 | 42
 (8 rows)
 
+--
+-- variant where a PlaceHolderVar is needed at a join, but not above the join
+--
+explain (costs off)
+select * from
+  int4_tbl as i41,
+  lateral
+    (select 1 as x from
+      (select i41.f1 as lat,
+              i42.f1 as loc from
+         int8_tbl as i81, int4_tbl as i42) as ss1
+      right join int4_tbl as i43 on (i43.f1 > 1)
+      where ss1.loc = ss1.lat) as ss2
+where i41.f1 > 0;
+                    QUERY PLAN                    
+--------------------------------------------------
+ Nested Loop
+   ->  Nested Loop
+         ->  Seq Scan on int4_tbl i41
+               Filter: (f1 > 0)
+         ->  Nested Loop
+               Join Filter: (i41.f1 = i42.f1)
+               ->  Seq Scan on int8_tbl i81
+               ->  Materialize
+                     ->  Seq Scan on int4_tbl i42
+   ->  Materialize
+         ->  Seq Scan on int4_tbl i43
+               Filter: (f1 > 1)
+(12 rows)
+
+select * from
+  int4_tbl as i41,
+  lateral
+    (select 1 as x from
+      (select i41.f1 as lat,
+              i42.f1 as loc from
+         int8_tbl as i81, int4_tbl as i42) as ss1
+      right join int4_tbl as i43 on (i43.f1 > 1)
+      where ss1.loc = ss1.lat) as ss2
+where i41.f1 > 0;
+     f1     | x 
+------------+---
+     123456 | 1
+     123456 | 1
+     123456 | 1
+     123456 | 1
+     123456 | 1
+     123456 | 1
+     123456 | 1
+     123456 | 1
+     123456 | 1
+     123456 | 1
+ 2147483647 | 1
+ 2147483647 | 1
+ 2147483647 | 1
+ 2147483647 | 1
+ 2147483647 | 1
+ 2147483647 | 1
+ 2147483647 | 1
+ 2147483647 | 1
+ 2147483647 | 1
+ 2147483647 | 1
+(20 rows)
+
 --
 -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE
 --

diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index d687216618c83ed263ae574cbdb07d89fef813a4..4de24c19040154085b4a65d90e07a90cea83a907 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -905,6 +905,33 @@ where
   1 = (select 1 from int8_tbl t3 where ss.y is not null limit 1)
 order by 1,2;
 
+--
+-- variant where a PlaceHolderVar is needed at a join, but not above the join
+--
+
+explain (costs off)
+select * from
+  int4_tbl as i41,
+  lateral
+    (select 1 as x from
+      (select i41.f1 as lat,
+              i42.f1 as loc from
+         int8_tbl as i81, int4_tbl as i42) as ss1
+      right join int4_tbl as i43 on (i43.f1 > 1)
+      where ss1.loc = ss1.lat) as ss2
+where i41.f1 > 0;
+
+select * from
+  int4_tbl as i41,
+  lateral
+    (select 1 as x from
+      (select i41.f1 as lat,
+              i42.f1 as loc from
+         int8_tbl as i81, int4_tbl as i42) as ss1
+      right join int4_tbl as i43 on (i43.f1 > 1)
+      where ss1.loc = ss1.lat) as ss2
+where i41.f1 > 0;
+
 --
 -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE
 --