Re: BUG #18429: Inconsistent results on similar queries with join lateral - Mailing list pgsql-bugs

Richard Guo <guofenglinux@gmail.com> writes:
> I wondered that we could fix this issue by checking em_nullable_relids
> in generate_join_implied_equalities_normal when we determine if an EC
> member is computable at the join.  Then I realize that this is not easy
> to achieve without knowing the exact join(s) where the nulling would
> happen, which is exactly what the nullingrel stuff introduced in v16
> does.  So your proposed fix seems the right way to go.

Yeah.  The reason these queries work correctly in v16+ is that
get_baserel_parampathinfo calls generate_join_implied_equalities
with joinrelids that don't include the outer join's relid, so
the latter won't produce any join clauses that need to be evaluated
above the outer join.  But later, when build_joinrel_restrictlist
calls generate_join_implied_equalities, it does include the outer
join's relid, so we correctly produce and enforce the clause as a
filter clause at the outer join's plan level.

However, pre-v16, those two calls pass the exact same parameters
and necessarily get the exact same clauses.  We could only fix that
with an API change for generate_join_implied_equalities, which seems
dangerous in stable branches.  So I think filtering the clause list
in get_baserel_parampathinfo is the right direction for a solution
there.  We can make it cleaner than in my WIP patch though...

> Now that we've learned that join_clause_is_movable_into's heuristic
> about physically referencing the target rel can fail for EC-derived
> clauses, I'm kind of concerned that we may end up with duplicate clauses
> in the final plan, since we do not check EC-derived clauses against
> join_clause_is_movable_into in get_baserel_parampathinfo while we do in
> create_nestloop_path.  What if we have an EC-derived clause that in
> get_baserel_parampathinfo it is put into ppi_clauses while in
> create_nestloop_path it does not pass the movability checking?  Is it
> possible to occur, or is it just my illusion?

I'm not sure either, but it certainly seems like a hazard.  Also,
get_joinrel_parampathinfo is really depending on getting consistent
results from join_clause_is_movable_into to assign clauses to the
correct join level.  So after sleeping on it I think that "the
results of generate_join_implied_equalities should pass
join_clause_is_movable_into" is an invariant we don't really want to
let go of, meaning that it would be a good idea to fix equivclass.c
to ensure that's true in these child-rel corner cases.  That's not
very hard, requiring just a small hack in create_join_clause, as
attached.  It's not that much of a hack either because there are
other places in equivclass.c that force the relid sets for child
expressions to be more than what pull_varnos would conclude (search
for comments mentioning pull_varnos).

Having done that, we can add code in HEAD/v16 to assert that
join_clause_is_movable_into is true here, while in the older branches
we can use it to filter rather than klugily checking nullable_relids
directly.  So I end with the attached two patches.

I didn't include the new test case in the HEAD/v16 patch; since it
doesn't represent a live bug for those branches I felt like maybe
it's not worth the test cycles going forward.  But there's certainly
room for the opposite opinion.  What do you think?

            regards, tom lane

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index a619ff9177..1d6bedb399 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -1896,6 +1896,21 @@ create_join_clause(PlannerInfo *root,
                                                   rightem->em_relids),
                                         ec->ec_min_security);

+    /*
+     * If either EM is a child, force the clause's clause_relids to include
+     * the relid(s) of the child rel.  In normal cases it would already, but
+     * not if we are considering appendrel child relations with pseudoconstant
+     * translated variables (i.e., UNION ALL sub-selects with constant output
+     * items).  We must do this so that join_clause_is_movable_into() will
+     * think that the clause should be evaluated at the correct place.
+     */
+    if (leftem->em_is_child)
+        rinfo->clause_relids = bms_add_members(rinfo->clause_relids,
+                                               leftem->em_relids);
+    if (rightem->em_is_child)
+        rinfo->clause_relids = bms_add_members(rinfo->clause_relids,
+                                               rightem->em_relids);
+
     /* If it's a child clause, copy the parent's rinfo_serial */
     if (parent_rinfo)
         rinfo->rinfo_serial = parent_rinfo->rinfo_serial;
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index 0c125e42e8..e05b21c884 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -1560,6 +1560,7 @@ get_baserel_parampathinfo(PlannerInfo *root, RelOptInfo *baserel,
     ParamPathInfo *ppi;
     Relids        joinrelids;
     List       *pclauses;
+    List       *eqclauses;
     Bitmapset  *pserials;
     double        rows;
     ListCell   *lc;
@@ -1595,14 +1596,25 @@ get_baserel_parampathinfo(PlannerInfo *root, RelOptInfo *baserel,

     /*
      * Add in joinclauses generated by EquivalenceClasses, too.  (These
-     * necessarily satisfy join_clause_is_movable_into.)
+     * necessarily satisfy join_clause_is_movable_into; but in assert-enabled
+     * builds, let's verify that.)
      */
-    pclauses = list_concat(pclauses,
-                           generate_join_implied_equalities(root,
-                                                            joinrelids,
-                                                            required_outer,
-                                                            baserel,
-                                                            NULL));
+    eqclauses = generate_join_implied_equalities(root,
+                                                 joinrelids,
+                                                 required_outer,
+                                                 baserel,
+                                                 NULL);
+#ifdef USE_ASSERT_CHECKING
+    foreach(lc, eqclauses)
+    {
+        RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+
+        Assert(join_clause_is_movable_into(rinfo,
+                                           baserel->relids,
+                                           joinrelids));
+    }
+#endif
+    pclauses = list_concat(pclauses, eqclauses);

     /* Compute set of serial numbers of the enforced clauses */
     pserials = NULL;
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 9f39f4661a..06f89a6732 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -1851,6 +1851,21 @@ create_join_clause(PlannerInfo *root,
                                                   rightem->em_nullable_relids),
                                         ec->ec_min_security);

+    /*
+     * If either EM is a child, force the clause's clause_relids to include
+     * the relid(s) of the child rel.  In normal cases it would already, but
+     * not if we are considering appendrel child relations with pseudoconstant
+     * translated variables (i.e., UNION ALL sub-selects with constant output
+     * items).  We must do this so that join_clause_is_movable_into() will
+     * think that the clause should be evaluated at the correct place.
+     */
+    if (leftem->em_is_child)
+        rinfo->clause_relids = bms_add_members(rinfo->clause_relids,
+                                               leftem->em_relids);
+    if (rightem->em_is_child)
+        rinfo->clause_relids = bms_add_members(rinfo->clause_relids,
+                                               rightem->em_relids);
+
     /* Mark the clause as redundant, or not */
     rinfo->parent_ec = parent_ec;

diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index 3c75fd56f2..6e1c87a4e2 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -1300,6 +1300,7 @@ get_baserel_parampathinfo(PlannerInfo *root, RelOptInfo *baserel,
     ParamPathInfo *ppi;
     Relids        joinrelids;
     List       *pclauses;
+    List       *eqclauses;
     double        rows;
     ListCell   *lc;

@@ -1333,14 +1334,24 @@ get_baserel_parampathinfo(PlannerInfo *root, RelOptInfo *baserel,
     }

     /*
-     * Add in joinclauses generated by EquivalenceClasses, too.  (These
-     * necessarily satisfy join_clause_is_movable_into.)
+     * Add in joinclauses generated by EquivalenceClasses, too.  In principle
+     * these should always satisfy join_clause_is_movable_into; but if we are
+     * below an outer join the clauses might contain Vars that should only be
+     * evaluated above the join, so we have to check.
      */
-    pclauses = list_concat(pclauses,
-                           generate_join_implied_equalities(root,
-                                                            joinrelids,
-                                                            required_outer,
-                                                            baserel));
+    eqclauses = generate_join_implied_equalities(root,
+                                                 joinrelids,
+                                                 required_outer,
+                                                 baserel);
+    foreach(lc, eqclauses)
+    {
+        RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+
+        if (join_clause_is_movable_into(rinfo,
+                                        baserel->relids,
+                                        joinrelids))
+            pclauses = lappend(pclauses, rinfo);
+    }

     /* Estimate the number of rows returned by the parameterized scan */
     rows = get_parameterized_baserel_size(root, baserel, pclauses);
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 867c6d20cc..b356153900 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -5949,6 +5949,37 @@ select * from
  3 | 3
 (6 rows)

+-- check for generation of join EC conditions at wrong level (bug #18429)
+explain (costs off)
+select * from (
+  select arrayd.ad, coalesce(c.hundred, 0) as h
+  from unnest(array[1]) as arrayd(ad)
+  left join lateral (
+    select hundred from tenk1 where unique2 = arrayd.ad
+  ) c on true
+) c2
+where c2.h * c2.ad = c2.h * (c2.ad + 1);
+                                              QUERY PLAN
+-------------------------------------------------------------------------------------------------------
+ Nested Loop Left Join
+   Filter: ((COALESCE(tenk1.hundred, 0) * arrayd.ad) = (COALESCE(tenk1.hundred, 0) * (arrayd.ad + 1)))
+   ->  Function Scan on unnest arrayd
+   ->  Index Scan using tenk1_unique2 on tenk1
+         Index Cond: (unique2 = arrayd.ad)
+(5 rows)
+
+select * from (
+  select arrayd.ad, coalesce(c.hundred, 0) as h
+  from unnest(array[1]) as arrayd(ad)
+  left join lateral (
+    select hundred from tenk1 where unique2 = arrayd.ad
+  ) c on true
+) c2
+where c2.h * c2.ad = c2.h * (c2.ad + 1);
+ ad | h
+----+---
+(0 rows)
+
 -- check the number of columns specified
 SELECT * FROM (int8_tbl i cross join int4_tbl j) ss(a,b,c,d);
 ERROR:  join expression "ss" has 3 columns available but 4 columns specified
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 1113e98445..11a857041a 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2029,6 +2029,25 @@ select * from
    (select q1.v)
   ) as q2;

+-- check for generation of join EC conditions at wrong level (bug #18429)
+explain (costs off)
+select * from (
+  select arrayd.ad, coalesce(c.hundred, 0) as h
+  from unnest(array[1]) as arrayd(ad)
+  left join lateral (
+    select hundred from tenk1 where unique2 = arrayd.ad
+  ) c on true
+) c2
+where c2.h * c2.ad = c2.h * (c2.ad + 1);
+select * from (
+  select arrayd.ad, coalesce(c.hundred, 0) as h
+  from unnest(array[1]) as arrayd(ad)
+  left join lateral (
+    select hundred from tenk1 where unique2 = arrayd.ad
+  ) c on true
+) c2
+where c2.h * c2.ad = c2.h * (c2.ad + 1);
+
 -- check the number of columns specified
 SELECT * FROM (int8_tbl i cross join int4_tbl j) ss(a,b,c,d);