Horton

Thread

Commits

  1. Remove leftover code in deconstruct_distribute_oj_quals().

  1. A problem in deconstruct_distribute_oj_quals

    Richard Guo <guofenglinux@gmail.com> — 2023-02-07T03:07:08Z

    In deconstruct_distribute_oj_quals, when we've identified a commutable
left join which provides join clause with flexible semantics, we try to
generate multiple versions of the join clause.  Here we have the logic
that puts back any ojrelids that were removed from its min_righthand.

     /*
      * Put any OJ relids that were removed from min_righthand back into
      * ojscope, else distribute_qual_to_rels will complain.
      */
     ojscope = bms_join(ojscope, bms_intersect(sjinfo->commute_below,
                                               sjinfo->syn_righthand));

I doubt this is necessary.  It seems to me that all relids mentioned
within the join clause have already been contained in ojscope, which is
the union of min_lefthand and min_righthand.

I noticed this code because I came across a problem with a query as
below.

create table t (a int);

select t1.a from (t t1 left join t t2 on true) left join (t t3 left join t
t4 on t3.a = t4.a) on t2.a = t3.a;

When we deal with qual 't2.a = t3.a', deconstruct_distribute_oj_quals
would always add the OJ relid of t3/t4 into its required_relids, due to
the code above, which I think is wrong.  The direct consequence is that
we would miss the plan that joins t2 and t3 directly.

If we add unique constraint for 'a' and try the outer-join removal
logic, we would notice that the left join of t2/t3 cannot be removed
because its join qual is treated as pushed down due to the fact that its
required_relids exceed the scope of the join.  I think this is also not
correct.

So is it safe we remove that code?

Thanks
Richard

  2. Re: A problem in deconstruct_distribute_oj_quals

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-07T06:12:51Z

    Richard Guo <guofenglinux@gmail.com> writes:
> In deconstruct_distribute_oj_quals, when we've identified a commutable
> left join which provides join clause with flexible semantics, we try to
> generate multiple versions of the join clause.  Here we have the logic
> that puts back any ojrelids that were removed from its min_righthand.

>      /*
>       * Put any OJ relids that were removed from min_righthand back into
>       * ojscope, else distribute_qual_to_rels will complain.
>       */
>      ojscope = bms_join(ojscope, bms_intersect(sjinfo->commute_below,
>                                                sjinfo->syn_righthand));

> I doubt this is necessary.  It seems to me that all relids mentioned
> within the join clause have already been contained in ojscope, which is
> the union of min_lefthand and min_righthand.

Hmm ... that was needed at some point in the development of that
function, but maybe it isn't as the code stands now.  It does look
like the "this_ojscope" manipulations within the loop cover this.

> I noticed this code because I came across a problem with a query as
> below.

> create table t (a int);

> select t1.a from (t t1 left join t t2 on true) left join (t t3 left join t
> t4 on t3.a = t4.a) on t2.a = t3.a;

> When we deal with qual 't2.a = t3.a', deconstruct_distribute_oj_quals
> would always add the OJ relid of t3/t4 into its required_relids, due to
> the code above, which I think is wrong.  The direct consequence is that
> we would miss the plan that joins t2 and t3 directly.

I don't see any change in this query plan when I remove that code, so
I'm not sure you're explaining your point very well.

			regards, tom lane

  3. Re: A problem in deconstruct_distribute_oj_quals

    Richard Guo <guofenglinux@gmail.com> — 2023-02-07T08:08:21Z

    On Tue, Feb 7, 2023 at 2:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Richard Guo <guofenglinux@gmail.com> writes:
> > I noticed this code because I came across a problem with a query as
> > below.
>
> > create table t (a int);
>
> > select t1.a from (t t1 left join t t2 on true) left join (t t3 left join
> t
> > t4 on t3.a = t4.a) on t2.a = t3.a;
>
> > When we deal with qual 't2.a = t3.a', deconstruct_distribute_oj_quals
> > would always add the OJ relid of t3/t4 into its required_relids, due to
> > the code above, which I think is wrong.  The direct consequence is that
> > we would miss the plan that joins t2 and t3 directly.
>
> I don't see any change in this query plan when I remove that code, so
> I'm not sure you're explaining your point very well.


Sorry I didn't make myself clear.  The plan change may not be obvious
except when the cheapest path happens to be joining t2 and t3 first and
then joining with t4 afterwards.  Currently HEAD would not generate such
a path because the joinqual of t2/t3 always has the OJ relid of t3/t4 in
its required_relids.

To observe an obvious plan change, we can add unique constraint for 'a'
and look how outer-join removal works.

alter table t add unique (a);

-- with that code
# explain (costs off)
select t1.a from (t t1 left join t t2 on true) left join (t t3 left join t
t4 on t3.a = t4.a) on t2.a = t3.a;
                    QUERY PLAN
---------------------------------------------------
 Nested Loop Left Join
   ->  Seq Scan on t t1
   ->  Nested Loop Left Join
         ->  Seq Scan on t t2
         ->  Index Only Scan using t_a_key on t t3
               Index Cond: (a = t2.a)
(6 rows)


-- without that code
# explain (costs off)
select t1.a from (t t1 left join t t2 on true) left join (t t3 left join t
t4 on t3.a = t4.a) on t2.a = t3.a;
          QUERY PLAN
------------------------------
 Nested Loop Left Join
   ->  Seq Scan on t t1
   ->  Materialize
         ->  Seq Scan on t t2
(4 rows)

This is another side-effect of that code.  The joinqual of t2/t3 is
treated as being pushed down when we try to remove t2/t3, because its
required_relids, which incorrectly includes the OJ relid of t3/t4,
exceed the scope of the join.  This is not right.

Thanks
Richard

  4. Re: A problem in deconstruct_distribute_oj_quals

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-07T16:58:24Z

    Richard Guo <guofenglinux@gmail.com> writes:
> On Tue, Feb 7, 2023 at 2:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't see any change in this query plan when I remove that code, so
>> I'm not sure you're explaining your point very well.

> To observe an obvious plan change, we can add unique constraint for 'a'
> and look how outer-join removal works.

Ah.  Yeah, that's pretty convincing, especially since v15 manages to
find that optimization.  Pushed with a test case.

			regards, tom lane