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/87blfgqa4t.fsf@aurora.ydns.eu

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
 --