Fix over-optimistic updating of info about commutable outer joins.

make_outerjoininfo was set up to update SpecialJoinInfo's
commute_below, commute_above_l, commute_above_r fields as soon as
it found a pair of outer joins that look like they can commute.
However, this decision could be negated later in the same loop due
to finding an intermediate outer join that prevents commutation.
That left us with commute_xxx fields that were contradictory to the
join order restrictions expressed in min_lefthand/min_righthand.
The latter fields would keep us from actually choosing a bad join
order; but the inconsistent commute_xxx fields could bollix details
such as the varnullingrels values created for intermediate join
relation targetlists, ending in an assertion failure in setrefs.c.

To fix, wait till the end of make_outerjoininfo where we have
accurate values for min_lefthand/min_righthand, and then insert
only relids not present in those sets into the commute_xxx fields.

Per SQLSmith testing by Robins Tharakan.  Note that while Robins
bisected the failure to commit b448f1c8d, it's really the fault of
2489d76c4.  The outerjoin_delayed logic removed in the later commit
was keeping us from deciding that troublesome join pairs commute,
at least in the specific example seen here.

Discussion: https://postgr.es/m/CAEP4nAyAORgE8K_RHSmvWbE9UaChhjbEL1RrDU3neePwwRUB=A@mail.gmail.com

diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index c301e6dffc41f60c9d7e77ef853c7ed0aa155fa4..2d9aa85e591a740ae5b0341f65f2f5ebd5ee7fd2 100644 (file)
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -1358,6 +1358,8 @@ make_outerjoininfo(PlannerInfo *root,
    Relids      strict_relids;
    Relids      min_lefthand;
    Relids      min_righthand;
+   Relids      commute_below_l;
+   Relids      commute_below_r;
    ListCell   *l;
 
    /*
@@ -1445,7 +1447,14 @@ make_outerjoininfo(PlannerInfo *root,
 
    /*
     * Now check previous outer joins for ordering restrictions.
+    *
+    * commute_below_l and commute_below_r accumulate the relids of lower
+    * outer joins that we think this one can commute with.  These decisions
+    * are just tentative within this loop, since we might find an
+    * intermediate outer join that prevents commutation.  Surviving relids
+    * will get merged into the SpecialJoinInfo structs afterwards.
     */
+   commute_below_l = commute_below_r = NULL;
    foreach(l, root->join_info_list)
    {
        SpecialJoinInfo *otherinfo = (SpecialJoinInfo *) lfirst(l);
@@ -1458,6 +1467,7 @@ make_outerjoininfo(PlannerInfo *root,
         */
        if (otherinfo->jointype == JOIN_FULL)
        {
+           Assert(otherinfo->ojrelid != 0);
            if (bms_overlap(left_rels, otherinfo->syn_lefthand) ||
                bms_overlap(left_rels, otherinfo->syn_righthand))
            {
@@ -1465,9 +1475,8 @@ make_outerjoininfo(PlannerInfo *root,
                                               otherinfo->syn_lefthand);
                min_lefthand = bms_add_members(min_lefthand,
                                               otherinfo->syn_righthand);
-               if (otherinfo->ojrelid != 0)
-                   min_lefthand = bms_add_member(min_lefthand,
-                                                 otherinfo->ojrelid);
+               min_lefthand = bms_add_member(min_lefthand,
+                                             otherinfo->ojrelid);
            }
            if (bms_overlap(right_rels, otherinfo->syn_lefthand) ||
                bms_overlap(right_rels, otherinfo->syn_righthand))
@@ -1476,9 +1485,8 @@ make_outerjoininfo(PlannerInfo *root,
                                                otherinfo->syn_lefthand);
                min_righthand = bms_add_members(min_righthand,
                                                otherinfo->syn_righthand);
-               if (otherinfo->ojrelid != 0)
-                   min_righthand = bms_add_member(min_righthand,
-                                                  otherinfo->ojrelid);
+               min_righthand = bms_add_member(min_righthand,
+                                              otherinfo->ojrelid);
            }
            /* Needn't do anything else with the full join */
            continue;
@@ -1536,11 +1544,9 @@ make_outerjoininfo(PlannerInfo *root,
            {
                /* Identity 3 applies, so remove the ordering restriction */
                min_lefthand = bms_del_member(min_lefthand, otherinfo->ojrelid);
-               /* Add commutability markers to both SpecialJoinInfos */
-               otherinfo->commute_above_l =
-                   bms_add_member(otherinfo->commute_above_l, ojrelid);
-               sjinfo->commute_below =
-                   bms_add_member(sjinfo->commute_below, otherinfo->ojrelid);
+               /* Record the (still tentative) commutability relationship */
+               commute_below_l =
+                   bms_add_member(commute_below_l, otherinfo->ojrelid);
            }
        }
 
@@ -1589,11 +1595,9 @@ make_outerjoininfo(PlannerInfo *root,
                /* Identity 3 applies, so remove the ordering restriction */
                min_righthand = bms_del_member(min_righthand,
                                               otherinfo->ojrelid);
-               /* Add commutability markers to both SpecialJoinInfos */
-               otherinfo->commute_above_r =
-                   bms_add_member(otherinfo->commute_above_r, ojrelid);
-               sjinfo->commute_below =
-                   bms_add_member(sjinfo->commute_below, otherinfo->ojrelid);
+               /* Record the (still tentative) commutability relationship */
+               commute_below_r =
+                   bms_add_member(commute_below_r, otherinfo->ojrelid);
            }
        }
    }
@@ -1639,6 +1643,44 @@ make_outerjoininfo(PlannerInfo *root,
    sjinfo->min_lefthand = min_lefthand;
    sjinfo->min_righthand = min_righthand;
 
+   /*
+    * Now that we've identified the correct min_lefthand and min_righthand,
+    * any commute_below_l or commute_below_r relids that have not gotten
+    * added back into those sets (due to intervening outer joins) are indeed
+    * commutable with this one.  Update the derived data in the
+    * SpecialJoinInfos.
+    */
+   if (commute_below_l || commute_below_r)
+   {
+       Relids      commute_below;
+
+       /*
+        * Delete any subsequently-added-back relids (this is easier than
+        * maintaining commute_below_l/r precisely through all the above).
+        */
+       commute_below_l = bms_del_members(commute_below_l, min_lefthand);
+       commute_below_r = bms_del_members(commute_below_r, min_righthand);
+
+       /* Anything left? */
+       commute_below = bms_union(commute_below_l, commute_below_r);
+       if (!bms_is_empty(commute_below))
+       {
+           /* Yup, so we must update the data structures */
+           sjinfo->commute_below = commute_below;
+           foreach(l, root->join_info_list)
+           {
+               SpecialJoinInfo *otherinfo = (SpecialJoinInfo *) lfirst(l);
+
+               if (bms_is_member(otherinfo->ojrelid, commute_below_l))
+                   otherinfo->commute_above_l =
+                       bms_add_member(otherinfo->commute_above_l, ojrelid);
+               else if (bms_is_member(otherinfo->ojrelid, commute_below_r))
+                   otherinfo->commute_above_r =
+                       bms_add_member(otherinfo->commute_above_r, ojrelid);
+           }
+       }
+   }
+
    return sjinfo;
 }
 

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 9762952efd21dc76c8ae7700300d74a0a3eb8fd2..037c7d0d566315c60bf645bb4a21608d0c5870dc 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -4469,6 +4469,36 @@ left join
          One-Time Filter: false
 (5 rows)
 
+-- check handling of apparently-commutable outer joins with non-commutable
+-- joins between them
+explain (costs off)
+select 1 from
+  int4_tbl i4
+  left join int8_tbl i8 on i4.f1 is not null
+  left join (select 1 as a) ss1 on null
+  join int4_tbl i42 on ss1.a is null or i8.q1 <> i8.q2
+  right join (select 2 as b) ss2
+  on ss2.b < i4.f1;
+                        QUERY PLAN                         
+-----------------------------------------------------------
+ Nested Loop Left Join
+   ->  Result
+   ->  Nested Loop
+         ->  Nested Loop Left Join
+               Join Filter: NULL::boolean
+               Filter: (((1) IS NULL) OR (i8.q1 <> i8.q2))
+               ->  Nested Loop Left Join
+                     Join Filter: (i4.f1 IS NOT NULL)
+                     ->  Seq Scan on int4_tbl i4
+                           Filter: (2 < f1)
+                     ->  Materialize
+                           ->  Seq Scan on int8_tbl i8
+               ->  Result
+                     One-Time Filter: false
+         ->  Materialize
+               ->  Seq Scan on int4_tbl i42
+(16 rows)
+
 --
 -- test for appropriate join order in the presence of lateral references
 --

diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 3ef29960409541405e7c353ab0d3fe86590ce6b2..1f2b7f62f0fb9eb525774f1c7ebe4c473d0528dd 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1545,6 +1545,17 @@ left join
   where c.relkind = 'r'
 ) ss2 on false;
 
+-- check handling of apparently-commutable outer joins with non-commutable
+-- joins between them
+explain (costs off)
+select 1 from
+  int4_tbl i4
+  left join int8_tbl i8 on i4.f1 is not null
+  left join (select 1 as a) ss1 on null
+  join int4_tbl i42 on ss1.a is null or i8.q1 <> i8.q2
+  right join (select 2 as b) ss2
+  on ss2.b < i4.f1;
+
 --
 -- test for appropriate join order in the presence of lateral references
 --