Further fixes in qual nullingrel adjustment for outer join commutation.

One of the add_nulling_relids calls in deconstruct_distribute_oj_quals
added an OJ relid to too few Vars, while the other added it to too
many.  We should consider the syntactic structure not
min_left/righthand while deciding which Vars to decorate, and when
considering pushing up a lower outer join pursuant to transforming the
second form of OJ identity 3 to the first form, we only want to
decorate Vars coming from its LHS.

In a related bug, I realized that make_outerjoininfo was failing to
check a very basic property that's needed to apply OJ identity 3:
the syntactically-upper outer join clause can't refer to the lower
join's LHS.  This didn't break the join order restriction logic,
but it led to setting bogus commute_xxx bits, possibly resulting
in bogus nullingrel markings in modified quals.

Richard Guo and Tom Lane

Discussion: https://postgr.es/m/CAMbWs497CmBruMx1SOjepWEz+T5NWa4scqbdE9v7ZzSXqH_gQw@mail.gmail.com
Discussion: https://postgr.es/m/CAEP4nAx9C5gXNBfEA0JBfz7B+5f1Bawt-RWQWyhev-wdps8BZA@mail.gmail.com

diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index 904f710d5931a71649c84711b60d8d3ab72b32b2..2b2c6af9ef8019a1da34676265880dfd7fac6a5e 100644 (file)
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -1540,7 +1540,8 @@ make_outerjoininfo(PlannerInfo *root,
                        }
                        else if (jointype == JOIN_LEFT &&
                                         otherinfo->jointype == JOIN_LEFT &&
-                                        bms_overlap(strict_relids, otherinfo->min_righthand))
+                                        bms_overlap(strict_relids, otherinfo->min_righthand) &&
+                                        !bms_overlap(clause_relids, otherinfo->syn_lefthand))
                        {
                                /* Identity 3 applies, so remove the ordering restriction */
                                min_lefthand = bms_del_member(min_lefthand, otherinfo->ojrelid);
@@ -1985,12 +1986,18 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
                        /*
                         * When we are looking at joins above sjinfo, we are envisioning
                         * pushing sjinfo to above othersj, so add othersj's nulling bit
-                        * before distributing the quals.
+                        * before distributing the quals.  We should add it to Vars coming
+                        * from the current join's LHS: we want to transform the second
+                        * form of OJ identity 3 to the first form, in which Vars of
+                        * relation B will appear nulled by the syntactically-upper OJ
+                        * within the Pbc clause, but those of relation C will not.  (In
+                        * the notation used by optimizer/README, we're converting a qual
+                        * of the form Pbc to Pb*c.)
                         */
                        if (above_sjinfo)
                                quals = (List *)
                                        add_nulling_relids((Node *) quals,
-                                                                          othersj->min_righthand,
+                                                                          sjinfo->syn_lefthand,
                                                                           bms_make_singleton(othersj->ojrelid));
 
                        /* Compute qualscope and ojscope for this join level */
@@ -2041,12 +2048,16 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
                        /*
                         * Adjust qual nulling bits for next level up, if needed.  We
                         * don't want to put sjinfo's own bit in at all, and if we're
-                        * above sjinfo then we did it already.
+                        * above sjinfo then we did it already.  Here, we should mark all
+                        * Vars coming from the lower join's RHS.  (Again, we are
+                        * converting a qual of the form Pbc to Pb*c, but now we are
+                        * putting back bits that were there in the parser output and were
+                        * temporarily stripped above.)
                         */
                        if (below_sjinfo)
                                quals = (List *)
                                        add_nulling_relids((Node *) quals,
-                                                                          othersj->min_righthand,
+                                                                          othersj->syn_righthand,
                                                                           bms_make_singleton(othersj->ojrelid));
 
                        /* ... and track joins processed so far */

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 75f03fcf30c52eabbce416a0a8c5906e0aba002c..98d611412dfb562205d1fb26cf845bc0931a49a3 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -5068,6 +5068,31 @@ where current_user is not null;  -- this is to add a Result node
                ->  Seq Scan on int4_tbl i4
 (6 rows)
 
+-- and further discussion of bug #17781
+explain (costs off)
+select *
+from int8_tbl t1
+  left join (int8_tbl t2 left join onek t3 on t2.q1 > t3.unique1)
+    on t1.q2 = t2.q2
+  left join onek t4
+    on t2.q2 < t3.unique2;
+                   QUERY PLAN                    
+-------------------------------------------------
+ Nested Loop Left Join
+   Join Filter: (t2.q2 < t3.unique2)
+   ->  Nested Loop Left Join
+         Join Filter: (t2.q1 > t3.unique1)
+         ->  Hash Left Join
+               Hash Cond: (t1.q2 = t2.q2)
+               ->  Seq Scan on int8_tbl t1
+               ->  Hash
+                     ->  Seq Scan on int8_tbl t2
+         ->  Materialize
+               ->  Seq Scan on onek t3
+   ->  Materialize
+         ->  Seq Scan on onek t4
+(13 rows)
+
 -- check that join removal works for a left join when joining a subquery
 -- that is guaranteed to be unique by its GROUP BY clause
 explain (costs off)

diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 2e764cdd51d2ce8c332305d19fd4fcaee1d5812b..e50a769606938719791eeaf4e6aaaf416bc1b4bd 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1820,6 +1820,15 @@ from (select f1/2 as x from int4_tbl i4 left join a on a.id = i4.f1) ss1
      right join int8_tbl i8 on true
 where current_user is not null;  -- this is to add a Result node
 
+-- and further discussion of bug #17781
+explain (costs off)
+select *
+from int8_tbl t1
+  left join (int8_tbl t2 left join onek t3 on t2.q1 > t3.unique1)
+    on t1.q2 = t2.q2
+  left join onek t4
+    on t2.q2 < t3.unique2;
+
 -- check that join removal works for a left join when joining a subquery
 -- that is guaranteed to be unique by its GROUP BY clause
 explain (costs off)