Re: Assert !bms_overlap(joinrel->relids, required_outer)

Richard Guo <guofenglinux@gmail.com> writes:
> On Wed, Jun 28, 2023 at 6:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> For a real fix, I'm inclined to extend the loop that calculates
>> param_source_rels (in add_paths_to_joinrel) so that it also tracks
>> a set of incompatible relids that *must not* be present in the
>> parameterization of a proposed path.  This would basically include
>> OJ relids of OJs that partially overlap the target joinrel; maybe
>> we should also include the min RHS of such OJs.  Then we could
>> check that in try_nestloop_path.  I've not tried to code this yet.

> I went ahead and drafted a patch based on this idea.

Hmm.  This patch is the opposite of what I'd been imagining, because
I was thinking we needed to add OJs to param_incompatible_relids if
they were *not* already in the join, rather than if they were.
However, I tried it like that and while it did stop the assertion
failure, it also broke a bunch of other test cases that no longer
found the parameterized-nestloop plans they were supposed to find.
So clearly I just didn't have my head screwed on in the correct
direction yesterday.

However, given that what we need is to exclude parameterization
that depends on the currently-formed OJ, it seems to me we can do
it more simply and without any new JoinPathExtraData field,
as attached.  What do you think?

> * I think we need to check the incompatible relids also in
> try_hashjoin_path and try_mergejoin_path besides try_nestloop_path.

I think this isn't necessary, at least in my formulation.
Those cases will go through calc_non_nestloop_required_outer
which has

    /* neither path can require rels from the other */
    Assert(!bms_overlap(outer_paramrels, inner_path->parent->relids));
    Assert(!bms_overlap(inner_paramrels, outer_path->parent->relids));

In order to have a dependency on an OJ, a path would have to have
a dependency on at least one of the OJ's base relations too, so
I think these assertions show that the case won't arise.  (Of
course, if someone can trip one of these assertions, I'm wrong.)

            regards, tom lane

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index c2f211a60d..4b6ed6e312 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -698,6 +698,17 @@ try_nestloop_path(PlannerInfo *root,
     Relids        inner_paramrels = PATH_REQ_OUTER(inner_path);
     Relids        outer_paramrels = PATH_REQ_OUTER(outer_path);

+    /*
+     * If we are forming an outer join at this join, it's nonsensical to use
+     * an input path that uses the outer join as part of its parameterization.
+     * (This can happen despite our join order restrictions, since those apply
+     * to what is in an input relation not what its parameters are.)
+     */
+    if (extra->sjinfo && extra->sjinfo->ojrelid != 0 &&
+        (bms_is_member(extra->sjinfo->ojrelid, outer_paramrels) ||
+         bms_is_member(extra->sjinfo->ojrelid, inner_paramrels)))
+        return;
+
     /*
      * Paths are parameterized by top-level parents, so run parameterization
      * tests on the parent relids.
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 6917faec14..12b828fae3 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -5063,6 +5063,37 @@ select 1 from
 ----------
 (0 rows)

+explain (costs off)
+select 1 from tenk1 t1
+         join lateral
+           (select t1.unique1 from (select 1) foo offset 0) s1 on true
+         join
+            (select 1 from tenk1 t2
+                inner join tenk1 t3
+                 left join tenk1 t4 left join tenk1 t5 on t4.unique1 = 1
+                on t4.unique1 = 1 on false
+             where t3.unique1 = coalesce(t5.unique1,1)) as s2
+          on true;
+        QUERY PLAN
+--------------------------
+ Result
+   One-Time Filter: false
+(2 rows)
+
+select 1 from tenk1 t1
+         join lateral
+           (select t1.unique1 from (select 1) foo offset 0) s1 on true
+         join
+            (select 1 from tenk1 t2
+                inner join tenk1 t3
+                 left join tenk1 t4 left join tenk1 t5 on t4.unique1 = 1
+                on t4.unique1 = 1 on false
+             where t3.unique1 = coalesce(t5.unique1,1)) as s2
+          on true;
+ ?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 55080bec9a..38899ed3b9 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1751,6 +1751,29 @@ select 1 from
   on false,
   lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3;

+explain (costs off)
+select 1 from tenk1 t1
+         join lateral
+           (select t1.unique1 from (select 1) foo offset 0) s1 on true
+         join
+            (select 1 from tenk1 t2
+                inner join tenk1 t3
+                 left join tenk1 t4 left join tenk1 t5 on t4.unique1 = 1
+                on t4.unique1 = 1 on false
+             where t3.unique1 = coalesce(t5.unique1,1)) as s2
+          on true;
+
+select 1 from tenk1 t1
+         join lateral
+           (select t1.unique1 from (select 1) foo offset 0) s1 on true
+         join
+            (select 1 from tenk1 t2
+                inner join tenk1 t3
+                 left join tenk1 t4 left join tenk1 t5 on t4.unique1 = 1
+                on t4.unique1 = 1 on false
+             where t3.unique1 = coalesce(t5.unique1,1)) as s2
+          on true;
+
 --
 -- check a case in which a PlaceHolderVar forces join order
 --