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

[ sorry for slow response ]

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> On Thu, Mar 2, 2023 at 11:27 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> What's feeling like it might be the best thing is to go ahead and
>> syntactically convert to the second form of identity 3 as soon as
>> we've determined it's applicable, so that upper C Vars are always
>> marked with both OJ relids. Not sure how much work is involved
>> there.

> I'm thinking something that once we've determined identity 3 applies in
> make_outerjoininfo we record information about how to adjust relids for
> upper quals and store this info in all upper JoinTreeItems. Then
> afterwards in distribute_qual_to_rels we check all this info stored in
> current JoinTreeItem and adjust relids for the qual accordingly.

> The info we record should consist of two parts, target_relids and
> added_relids. Vars and PHVs in quals that belong to target_relids
> should be adjusted to include added_relids.

> Following this idea I come up with attached patch (no comments and test
> cases yet). It fixes the presented case and passes check-world. Before
> finishing it I'd like to know whether this idea works. Any comments are
> appreciated.

This doesn't look hugely promising to me. We do need to do something
like "Vars/PHVs referencing join X now need to also reference join Y",
but I think we need to actually change the individual Vars/PHVs not
just fake it by hacking the required_relids of RestrictInfos. Otherwise
we're going to get assertion failures in setrefs.c about nullingrel
sets not matching up.

Also, in addition to upper-level quals, we need to apply the same
transformation to the query's targetlist and havingQual if anyway
(and perhaps also mergeActionList, not sure).

So the idea that I had was to, once we detect that X commutes with Y,
immediately call a function that recurses through the relevant parts
of root->parse and performs the required nullingrels updates. This'd
result in multiple transformation traversals if there are several
commuting pairs of joins, but I doubt that that would really pose a
performance problem. If it does, we could imagine splitting up
deconstruct_jointree still further, so that detection of commutable
OJs happens in a pass earlier than distribute_quals_to_rels, and then
we fix up Vars/PHVs throughout the tree in one scan done between those
passes. But an extra deconstruct recursion pass isn't free either, so
I don't want to do that unless we actually see a performance problem.
Assuming we don't do that but perform this transformation during
deconstruct_jointree phase 2, it'd be too late to modify quals of
already-processed join tree nodes, but that's fine because they
couldn't contain Vars needing adjustment. (We could exploit that
in an incomplete way by passing in the address of the current
JoinExpr, and not bothering to recurse below it.) There is code
roughly like this in prepjointree.c already --- in particular, we'd
need to steal its hack of mutating join quals but not the JoinExpr and
FromExpr nodes themselves, to avoid breaking the recursion-in-progress.

I can have a go at coding this, or you can if you'd like to.

regards, tom lane