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

On Wed, May 17, 2023 at 11:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Ah, good idea.  I made some cosmetic adjustments and pushed the lot.


Thanks for pushing it!


> We still have a couple of loose ends in this thread:
>
> 1. Do we need the change in add_nulling_relids that I postulated in [1]?
>
> -                       bms_overlap(phv->phrels, context->target_relids))
> +                       bms_is_subset(phv->phrels, context->target_relids))
>
> The test case that made that fall over no longer does so, but I'm
> suspicious that this is still needed.  I'm not quite sure though,
> and I'm even less sure about the corresponding change in
> remove_nulling_relids:
>
> -                       !bms_overlap(phv->phrels, context->except_relids))
> +                       !bms_is_subset(phv->phrels,
> context->except_relids))
>
> which I made up pretty much out of whole cloth.


I'm not sure either.  I cannot think of a scenario where this change
would make a difference.  But I think this change is good to have.  It
is more consistent with how we check if a PHV belongs to a relid set in
build_joinrel_tlist(), where we use

    bms_is_subset(phv->phrels, sjinfo->syn_righthand)

to check if the PHV comes from within the syntactically nullable side of
the outer join.


> 2. Can we do anything to tighten make_outerjoininfo's computation of
> potential commutator pairs, as I speculated in [2]?  This isn't a bug
> hazard anymore (I think), but adding useless bits there clearly does
> cause us to waste cycles later.  If we can tighten that cheaply, it
> should be a win.


Agreed that we should try our best to get rid of non-commutable outer
joins from commute_below_l.  Not sure how to do that currently.  Maybe
we can add a TODO item for now?


> 3. In general, it seems like what your fixes are doing is computing
> transitive closures of the commute_above relationships.  I wonder if
> we can save anything by pre-calculating the closure sets.  Again,
> I don't want to add cycles in cases where the whole thing doesn't
> apply, but maybe we'd not have to.


Maybe we can pre-calculate the commute_above closure sets in
make_outerjoininfo and save it in SpecialJoinInfo?  I haven't thought
through this.

BTW, it seems that there is a minor thinko in the changes.  In the
outer-join removal logic, we use syn_xxxhand to compute the relid set
for the join we are considering to remove.  I think this might be not
right, because the outer joins may not be performed in syntactic order.
Consider the query below

create table t (a int unique, b int);

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

Attached is a patch to revert this logic to what it looked like before.

Thanks
Richard