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

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> On Sat, May 13, 2023 at 1:25 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Here's a patch series that does it that way.

> I explored this approach before and found that there are several
> problems with it that we need to resolve to make it work [1]. I
> reviewed the patches here and it seems that the second problem still
> exists. Quote it here.

Yeah, I realized over the weekend that I'd forgotten to update
deconstruct_distribute_oj_quals, and that the patch was missing some
potential join orders. Notably, the case that I thought was a performance
improvement turned out to be a mirage. It appears that the reason the
planner doesn't find the lowest-cost plan there is that it's following
the heuristic that it should avoid applying clause-free joins when
possible, so in that case it wants to put off the join to t1 as long
as possible. The bug forced it to make that join earlier which turned
out to be good. (I suppose this calls that whole heuristic into question;
but a few days before beta is no time to start revisiting heuristics
we've used for decades.)

> Also, it seems that PHVs in outer join's target list are not handled
> correctly, check the query below which causes assertion failure in
> search_indexed_tlist_for_phv.

Good catch! I think the problem here is that add_nulling_relids() is
being too aggressive: it's adding a nullingrel to a PHV when that's
nonsensical because the PHV is required to be computed above that join.
I fixed it with

- bms_overlap(phv->phrels, context->target_relids))
+ bms_is_subset(phv->phrels, context->target_relids))

which I think is sufficient, though if it proves not to be maybe we could
instead remove phv->phrels from the updated phnullingrels.

> BTW, there is a typo in the 0001's subject. It should be
> SpecialJoinInfo.commute_below rather than RestrictInfo.commute_below.

D'oh. Here's a patchset with these issues addressed.

regards, tom lane