Re: plan shape work

On Fri, Sep 12, 2025 at 11:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I was arguing for labeling plan nodes according to which OJ they
> start (always a unique relid).  Richard was arguing for labeling
> according to which OJ(s) they complete (zero, one, or more relids).

I agree with everything in your reply up to and including this part.

> But I now think it's probably worth doing both.  We need the
> completion bitmapsets if we want to cross-check Var nullingrels,
> because those correspond to the nullingrels that should get added
> at each join's output.  I think that we also want the start labels
> though.  For one thing, if the start nodes are not identified,
> it's impossible to understand how much of the tree is the "no
> man's land" where a C variable may or may not have gone to null
> on its way to becoming a C* variable.  But in general I think
> that we'll want to be able to identify an outer-join plan node
> even if it does not complete its OJ.

So, it looks to me like the way this works today is that
join_is_legal() figures out the relevant SpecialJoinInfo and then
add_outer_joins_to_relids() decides what to add to the joinrel's relid
set. So I think, though I am not quite sure, that if somewhere around
that point in the code we copied sjinfo->ojrelid into the RelOptInfo,
and then propagated that through to the final plan, that might be what
you're looking for here. However, that assumes that the choice of
SpecialJoinInfo is fixed for all possible ways of constructing a given
joinrel, which I think might not be true. In Richard's test case, one
simply can't go wrong, because the lower join only draws from two
baserels. But in a case like A LJ B ON A.x = B.x LJ C ON B.y = C.y LJ
D ON B.z = D.z, the B-C-D joinrel could presumably be constructed by
joining either B-D to C or B-C to D, and I'm guessing that will result
in a different choice of SpecialJoinInfo in each case. That would mean
that the ojrelid has to be per-Path rather than per-RelOptInfo.

While in theory that's fine, it sounds expensive. I was hoping that we
could piggyback mostly on existing calculations here, exposing data we
already have instead of calculating new things. If we want to expose
the starting-the-outer-join-RTI value that you want here, it seems
like we're going to have to redo some of the join_is_legal() work and
some of the add_outer_joins_to_relids() work for every path. I'm
skeptical about expending those cycles. It's not clear to me that I
need to care about outer join RTIs at all for what I'm trying to do --
focusing where RTIs originating from baserels end up in the final plan
tree is, as far as I can see, completely adequate. There might be
other people who want to do other things that would benefit more from
seeing that stuff, though. I'm not against exposing information that
is easily calculated and might be useful to somebody, even if it's
just for planner debugging. But it seems to me that what you're asking
here might be going quite a bit further than that.

So my counter-proposal is that this patch set should either (1) expose
nothing at all about join RTIs because I don't have a need for them or
(2) expose the join RTIs completed at a certain level because that's
easily calculated from the data we already have; and if you want to
later also expose the single join RTI started at a certain level for a
varnullingrels cross-check or any other purpose, then you can propose
a patch for that. Alternatively, if you want to edit my 0003 patch to
work the way you think it should, cool.  Or if you can describe what
you think it should do, I'm somewhat willing to try implementing that,
but that's definitely not such a great choice from my perspective. I'm
afraid that I'm getting pulled a bit further down the garden path than
I really want to go trying to satisfy your desire to perform a
cross-check that I don't really understand and/or expose information
for which I don't see a clear need in my own work. What I need is for
all the baserels that appear in the final Plan tree to be properly
labelled.

--
Robert Haas
EDB: http://www.enterprisedb.com