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

On Sat, Apr 1, 2023 at 10:15 AM Richard Guo <guofenglinux@gmail.com> wrote:

> On Wed, Mar 1, 2023 at 2:44 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
>> On Wed, Mar 1, 2023 at 3:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>>> Here's said patch.  Although this fixes the described problem and
>>> passes check-world, I'm not totally happy with it yet: it feels
>>> like the new add_outer_joins_to_relids() function is too expensive
>>> to be doing every time we construct a join relation.
>>
>> Do we need to revise how we build target list for outer join by
>> adjusting the nullingrels of Vars and PHVs from input_rel in a similar
>> way?
>>
>
> After more thought on this idea I think it's more promising, as long as
> we do proper adjustments to outer join's target list.  AFAICS we need to
> do two things for the adjustment.
>
> * we need to check whether the outer join has been completely performed
> before we add its relid to the nulling bitmap.  For example, the pushed
> down B/C join should not mark C Vars as nulled by itself.  This can be
> done by bms_is_member(sjinfo->ojrelid, joinrel->relids).
>
> * for all the pushed down outer joins that have been completely
> performed just by now, we need to add their relids to the nulling bitmap
> if they can null the Var or PHV.  For example, when we form the pulled
> up A/B join, we need to mark C Vars as nulled by the pushed down B/C
> join.  To do this, I'm considering we can collect all the pushed down
> outer joins in add_outer_joins_to_relids().
>
> Attached is the patch to do adjustments to outer join's target list.
> Note that it needs to be applied on the base of patch
> 'postpone-adding-pushed-down-ojrelid-to-relids.patch'.
>

I found a thinko in the v1 patch: we need to consider syn_righthand
rather than min_righthand of the pushed down outer joins as we want to
know if the Var or PHV actually comes from within the syntactically
nullable sides.  Attach v2 to fix this.

I have some concerns that the new added foreach loop is too expensive to
be doing for each Var or PHV.  But maybe it's no problem since we only
loop over pushed down joins here.

Thanks
Richard