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(at)sss(dot)pgh(dot)pa(dot)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