Fix thinkos in have_unsafe_outer_join_ref; reduce to Assert check.

Late in the development of commit 2489d76c4, I (tgl) incorrectly
concluded that the new function have_unsafe_outer_join_ref couldn't
ever reach its inner loop.  That should be the case if the inner
rel's parameterization is based on just one Var, but it could be
based on Vars from several relations, and then not only is the
inner loop reachable but it's wrongly coded.

Despite those errors, it still appears that the whole thing is
redundant given previous join_is_legal checks, so let's arrange
to only run it in assert-enabled builds.

Diagnosis and patch by Richard Guo, per fuzz testing by Justin Pryzby.

Discussion: https://postgr.es/m/20230212235823.GW1653@telsasoft.com

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 9d4a9197ee6ed573eea7ac0dc9f8fd926adde4da..e6ef0deb234923f7701edb16251f1895f9afea84 100644 (file)
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -378,9 +378,9 @@ allow_star_schema_join(PlannerInfo *root,
  * restrictions prevent us from attempting a join that would cause a problem.
  * (That's unsurprising, because the code worked before we ever added
  * outer-join relids to expression relids.)  It still seems worth checking
- * as a backstop, but we don't go to a lot of trouble: just reject if the
- * unsatisfied part includes any outer-join relids at all.
+ * as a backstop, but we only do so in assert-enabled builds.
  */
+#ifdef USE_ASSERT_CHECKING
 static inline bool
 have_unsafe_outer_join_ref(PlannerInfo *root,
                                                   Relids outerrelids,
@@ -388,11 +388,10 @@ have_unsafe_outer_join_ref(PlannerInfo *root,
 {
        bool            result = false;
        Relids          unsatisfied = bms_difference(inner_paramrels, outerrelids);
+       Relids          satisfied = bms_intersect(inner_paramrels, outerrelids);
 
-       if (unlikely(bms_overlap(unsatisfied, root->outer_join_rels)))
+       if (bms_overlap(unsatisfied, root->outer_join_rels))
        {
-#ifdef NOT_USED
-               /* If we ever weaken the join order restrictions, we might need this */
                ListCell   *lc;
 
                foreach(lc, root->join_info_list)
@@ -401,25 +400,23 @@ have_unsafe_outer_join_ref(PlannerInfo *root,
 
                        if (!bms_is_member(sjinfo->ojrelid, unsatisfied))
                                continue;               /* not relevant */
-                       if (bms_overlap(inner_paramrels, sjinfo->min_righthand) ||
+                       if (bms_overlap(satisfied, sjinfo->min_righthand) ||
                                (sjinfo->jointype == JOIN_FULL &&
-                                bms_overlap(inner_paramrels, sjinfo->min_lefthand)))
+                                bms_overlap(satisfied, sjinfo->min_lefthand)))
                        {
                                result = true;  /* doesn't work */
                                break;
                        }
                }
-#else
-               /* For now, if we do see an overlap, just assume it's trouble */
-               result = true;
-#endif
        }
 
        /* Waste no memory when we reject a path here */
        bms_free(unsatisfied);
+       bms_free(satisfied);
 
        return result;
 }
+#endif                                                 /* USE_ASSERT_CHECKING */
 
 /*
  * paraminfo_get_equal_hashops
@@ -713,16 +710,15 @@ try_nestloop_path(PlannerInfo *root,
        /*
         * Check to see if proposed path is still parameterized, and reject if the
         * parameterization wouldn't be sensible --- unless allow_star_schema_join
-        * says to allow it anyway.  Also, we must reject if either
-        * have_unsafe_outer_join_ref or have_dangerous_phv don't like the look of
-        * it, which could only happen if the nestloop is still parameterized.
+        * says to allow it anyway.  Also, we must reject if have_dangerous_phv
+        * doesn't like the look of it, which could only happen if the nestloop is
+        * still parameterized.
         */
        required_outer = calc_nestloop_required_outer(outerrelids, outer_paramrels,
                                                                                                  innerrelids, inner_paramrels);
        if (required_outer &&
                ((!bms_overlap(required_outer, extra->param_source_rels) &&
                  !allow_star_schema_join(root, outerrelids, inner_paramrels)) ||
-                have_unsafe_outer_join_ref(root, outerrelids, inner_paramrels) ||
                 have_dangerous_phv(root, outerrelids, inner_paramrels)))
        {
                /* Waste no memory when we reject a path here */
@@ -730,6 +726,9 @@ try_nestloop_path(PlannerInfo *root,
                return;
        }
 
+       /* If we got past that, we shouldn't have any unsafe outer-join refs */
+       Assert(!have_unsafe_outer_join_ref(root, outerrelids, inner_paramrels));
+
        /*
         * Do a precheck to quickly eliminate obviously-inferior paths.  We
         * calculate a cheap lower bound on the path's cost and then use

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 2f1f8b8dbe6730e1a1255cbedd0d1afc19a084c0..db9512969f37396ec83bcb7fd45d4d18554669c8 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -4653,6 +4653,32 @@ where tt1.f1 = ss1.c0;
 ----------
 (0 rows)
 
+explain (verbose, costs off)
+select 1 from
+  int4_tbl as i4
+  inner join
+    ((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1
+     right join (select 1 as z) as ss2 on true)
+  on false,
+  lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3;
+        QUERY PLAN        
+--------------------------
+ Result
+   Output: 1
+   One-Time Filter: false
+(3 rows)
+
+select 1 from
+  int4_tbl as i4
+  inner join
+    ((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1
+     right join (select 1 as z) as ss2 on true)
+  on false,
+  lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3;
+ ?column? 
+----------
+(0 rows)
+
 --
 -- check a case in which a PlaceHolderVar forces join order
 --

diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 400c16958f31465576e5004f9604af852ec120e4..2ff68879d228770f91f7ddfe07a941eb8d10c4dd 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1609,6 +1609,23 @@ select 1 from
   lateral (select tt4.f1 as c0 from text_tbl as tt5 limit 1) as ss1
 where tt1.f1 = ss1.c0;
 
+explain (verbose, costs off)
+select 1 from
+  int4_tbl as i4
+  inner join
+    ((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1
+     right join (select 1 as z) as ss2 on true)
+  on false,
+  lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3;
+
+select 1 from
+  int4_tbl as i4
+  inner join
+    ((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1
+     right join (select 1 as z) as ss2 on true)
+  on false,
+  lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3;
+
 --
 -- check a case in which a PlaceHolderVar forces join order
 --