Re: Clause accidentally pushed down ( Possible bug in Making Vars outer-join aware)

I'm reviewing the v5 patches and I find that the following change in
deconstruct_distribute_oj_quals is suspicious.

   if (joins_below)
   {
       /* Reset serial counter for this version of the quals */
       root->last_rinfo_serial = save_last_rinfo_serial;

       /*
        * Add lower joins' relids to the qual.  We should add them 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.)
        */
       quals = (List *)
           add_nulling_relids((Node *) quals,
                              sjinfo->syn_lefthand,
                              joins_below);

I doubt this is always right to add joins_below to all the vars
belonging to sjinfo->syn_lefthand.  What if the joins in joins_below
cannot commute with each other?  As a counterexample, consider the query
below which causes assertion failure in search_indexed_tlist_for_var.
The query is designed so that t1/t2 join cannot commute with t2/t3 join
but can commute with t3/t4 join.

explain (costs off)
select * from t t1
    left join t t2 on true
    left join t t3 on true
    left join t t4 on t2.a = t3.a;
server closed the connection unexpectedly

Also, it seems that this logic may cause us to miss join quals in the
final plan.  Consider the query below.

explain (costs off)
select * from t t1
    left join t t2 on true
    left join t t3 on t2.a = t3.a
    left join t t4 on t3.a != t4.a;
                   QUERY PLAN
------------------------------------------------
 Nested Loop Left Join
   ->  Seq Scan on t t1
   ->  Materialize
         ->  Nested Loop Left Join
               ->  Hash Left Join
                     Hash Cond: (t2.a = t3.a)
                     ->  Seq Scan on t t2
                     ->  Hash
                           ->  Seq Scan on t t3
               ->  Materialize
                     ->  Seq Scan on t t4
(11 rows)

So we've missed join qual 't3.a != t4.a' in this plan shape.  For this
join qual , deconstruct_distribute_oj_quals() generated two versions,
one with empty nullingrels, the other with nullingrels {t1/t2, t2/t3}.
Both are not applicable at join level (t2/t3)/t4.  I think we should
have another version with nullingrels {t2/t3} for this qual.

Thanks
Richard