Re: Making Vars outer-join aware

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> When we add required PlaceHolderVars to a join rel's targetlist, if the
> PHV can be computed in the nullable-side of the join, we would add the
> join's RT index to phnullingrels. This is correct as we know the PHV's
> value can be nulled at this join. But I'm wondering if it is necessary
> since we have already propagated any varnullingrels into the PHV when we
> apply pullup variable replacement in perform_pullup_replace_vars().

I'm not seeing the connection there? Any nullingrels that are set
during perform_pullup_replace_vars would refer to outer joins within the
pulled-up subquery, whereas what you are talking about here is what
happens when the PHV's value propagates up through outer joins of the
parent query. There's no overlap between those relid sets, or if there
is we have some fault in the logic that constrains join order to ensure
that there's a valid place to compute each PHV.

> On the other hand, the phnullingrels may contain RT indexes of outer
> joins above this join level. It seems not good to add such a PHV to the
> joinrel's targetlist.

Hmm, yeah, add_placeholders_to_joinrel is doing this wrong. The
intent was to match what build_joinrel_tlist does with plain Vars,
but in that case we're adding the join's relid to what we find in
varnullingrels in the input tlist. Using the phnullingrels from
the placeholder_list entry is wrong. (I wonder whether a
placeholder_list entry's phnullingrels is meaningful at all?
Maybe it isn't.) I think it might work to take the intersection
of the join's relids with root->outer_join_rels.

> If the two issues are indeed something we need to fix, maybe we can
> change add_placeholders_to_joinrel() to search the PHVs from
> outer_rel/inner_rel's targetlist, and add the ojrelid to phnullingrels
> if needed, just like what we do in build_joinrel_tlist(). The PHVs there
> should have the correct phnullingrels (at least the PHVs in base rels'
> targetlists have correctly set phnullingrels to NULL).

Yeah, maybe we should do something more invasive and make use of the
input targetlists rather than doing this from scratch. Not sure.
I'm worried that doing it that way would increase the risk of getting
different join tlist contents depending on which pair of input rels
we happen to consider first.

regards, tom lane