Re: Assert failure of the cross-check for nullingrels

Richard Guo <guofenglinux@gmail.com> writes:
> On Thu, May 18, 2023 at 3:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> After some poking at it I hit on what seems like a really simple
>> solution: we should be checking syn_righthand not min_righthand
>> to see whether a Var should be considered nullable by a given OJ.

> I thought about this solution before but proved it was not right in
> https://www.postgresql.org/message-id/CAMbWs48fObJJ%3DYVb4ip8tnwxwixUNKUThfnA1eGfPzJxJRRgZQ%40mail.gmail.com
> I checked the query shown there and it still fails with v3 patch.

Bleah.  The other solution I'd been poking at involved adding an
extra check for clone clauses, as attached (note this requires
8a2523ff3).  This survives your example, but I wonder if it might
reject all the clones in some cases.  It seems a bit expensive
too, although as I said before, I don't think the clone cases get
traversed all that often.

Perhaps another answer could be to compare against syn_righthand
for clone clauses and min_righthand for non-clones?  That seems
mighty unprincipled though.

            regards, tom lane

diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index d2bc096e1c..2071783987 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -532,6 +532,12 @@ extract_actual_join_clauses(List *restrictinfo_list,
  * This function checks the second condition; we assume the caller already
  * saw to the first one.
  *
+ * For has_clone and is_clone clauses, there is a third condition: the
+ * clause_relids and eval_relids must list the same subset of relevant
+ * commutating outer joins.  This ensures we don't select the wrong one
+ * of the clones.  (We do not check this for non-cloned clauses, as it
+ * might improperly reject them.)
+ *
  * For speed reasons, we don't individually examine each Var/PHV of the
  * expression, but just look at the overall clause_relids (the union of the
  * varnos and varnullingrels).  This could give a misleading answer if the
@@ -561,7 +567,27 @@ clause_is_computable_at(PlannerInfo *root,

         /* OK if clause lists it (we assume all Vars in it agree). */
         if (bms_is_member(sjinfo->ojrelid, clause_relids))
-            continue;
+        {
+            if (rinfo->has_clone || rinfo->is_clone)
+            {
+                /* Must check whether it's the right one of the clones */
+                Relids        commutators = bms_union(sjinfo->commute_below_l,
+                                                    sjinfo->commute_above_r);
+                Relids        clause_commutators = bms_intersect(clause_relids,
+                                                               commutators);
+                Relids        eval_commutators = bms_intersect(eval_relids,
+                                                             commutators);
+                bool        match = bms_equal(clause_commutators,
+                                              eval_commutators);
+
+                bms_free(commutators);    /* be tidy */
+                bms_free(clause_commutators);
+                bms_free(eval_commutators);
+                if (!match)
+                    return false;    /* wrong clone */
+            }
+            continue;            /* OK, clause expects this OJ to be done */
+        }

         /* Else, trouble if clause mentions any nullable Vars. */
         if (bms_overlap(clause_relids, sjinfo->min_righthand) ||
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 9bafadde66..b254a1b649 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -2500,6 +2500,52 @@ select * from int4_tbl t1
                      ->  Seq Scan on int4_tbl t4
 (12 rows)

+explain (costs off)
+select * from int4_tbl t1
+  left join (int4_tbl t2 left join int4_tbl t3 on t2.f1 > 0) on t2.f1 > 1
+  left join int4_tbl t4 on t2.f1 > 2 and t3.f1 > 3
+where t1.f1 = coalesce(t2.f1, 1);
+                     QUERY PLAN
+----------------------------------------------------
+ Nested Loop Left Join
+   Join Filter: ((t2.f1 > 2) AND (t3.f1 > 3))
+   ->  Nested Loop Left Join
+         Join Filter: (t2.f1 > 0)
+         ->  Nested Loop Left Join
+               Filter: (t1.f1 = COALESCE(t2.f1, 1))
+               ->  Seq Scan on int4_tbl t1
+               ->  Materialize
+                     ->  Seq Scan on int4_tbl t2
+                           Filter: (f1 > 1)
+         ->  Seq Scan on int4_tbl t3
+   ->  Materialize
+         ->  Seq Scan on int4_tbl t4
+(13 rows)
+
+explain (costs off)
+select * from int4_tbl t1
+  left join ((select t2.f1 from int4_tbl t2
+                left join int4_tbl t3 on t2.f1 > 0
+                where t3.f1 is null) s
+             left join tenk1 t4 on s.f1 > 1)
+    on s.f1 = t1.f1;
+                   QUERY PLAN
+-------------------------------------------------
+ Nested Loop Left Join
+   Join Filter: (t2.f1 > 1)
+   ->  Hash Right Join
+         Hash Cond: (t2.f1 = t1.f1)
+         ->  Nested Loop Left Join
+               Join Filter: (t2.f1 > 0)
+               Filter: (t3.f1 IS NULL)
+               ->  Seq Scan on int4_tbl t2
+               ->  Materialize
+                     ->  Seq Scan on int4_tbl t3
+         ->  Hash
+               ->  Seq Scan on int4_tbl t1
+   ->  Seq Scan on tenk1 t4
+(13 rows)
+
 --
 -- check a case where we formerly got confused by conflicting sort orders
 -- in redundant merge join path keys
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index a44234b0af..a9352bf4e0 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -488,6 +488,20 @@ select * from int4_tbl t1
   left join int4_tbl t3 on t2.f1 = t3.f1
   left join int4_tbl t4 on t3.f1 != t4.f1;

+explain (costs off)
+select * from int4_tbl t1
+  left join (int4_tbl t2 left join int4_tbl t3 on t2.f1 > 0) on t2.f1 > 1
+  left join int4_tbl t4 on t2.f1 > 2 and t3.f1 > 3
+where t1.f1 = coalesce(t2.f1, 1);
+
+explain (costs off)
+select * from int4_tbl t1
+  left join ((select t2.f1 from int4_tbl t2
+                left join int4_tbl t3 on t2.f1 > 0
+                where t3.f1 is null) s
+             left join tenk1 t4 on s.f1 > 1)
+    on s.f1 = t1.f1;
+
 --
 -- check a case where we formerly got confused by conflicting sort orders
 -- in redundant merge join path keys