Re: remove_useless_joins vs. bug #19560

I spent some time looking into what would be involved in back-patching
this. Quite aside from risks of new bugs, there's an API/ABI break
involved for released branches (I don't think we are worried about
that yet for v19, though). One thing we can do to reduce the API risk
is to not delete the no-longer-used extern functions that the master
patch removes, such as add_vars_to_attr_needed. I kind of doubt that
anyone is using those in extensions, but leaving them as dead code
seems like cheap insurance. However, there's an irreducible minimum
API change in remove_useless_joins, reduce_unique_semijoins, and
remove_useless_self_joins: those don't return what they did before,
and even more importantly their side effects are much different from
before. I don't see any evidence in Debian Code Search that anyone is
calling those, so I'm not inclined to go to lengths like (say) keeping
the whole of the existing analyzejoins.c code around as dead code.
Even if we wanted to consider that, the bugs that we're trying to get
rid of would still bite any hypothetical extension that was messing
with these functions. So I think it's reasonable to trust that nobody
is calling anything lower-level than query_planner().

Having said that, I think that it'd be a good idea to rename
remove_useless_joins to remove_useless_outer_joins. The existing
name is already confusing now that we have remove_useless_self_joins
too. The ABI angle is that if we do that, then any extension that's
trying to call remove_useless_joins will fail with an
easily-understood error at library load time, rather than crashing
or misbehaving in some arcane way because the function didn't do what
it expected. We could extend this to renaming all three functions,
but I think renaming remove_useless_joins is sufficient --- I really
don't see any use case for calling the other two and not that one.

So attached are a set of patches along that line. v5-0001 is the
master patchset squashed to one patch; it differs from the v4 series
in having the remove_useless_joins rename and some minor additional
comment-smithing. The others are draft patches for v16-v19. The
v19 patch is only trivially different from master. v18 and earlier
don't remove any of the dead code outside analyzejoins.c. v16/v17
are a good deal smaller because there's no SJE code to fix.

I think it's still open to debate how far back we want to go.
In principle we could make these changes in v14/v15, but with no known
bugs manifesting in those branches, the risk/reward ratio seems poor,
so I didn't make patches for those. I think we definitely want to do
this in v18, since aside from #19560 that branch has the SJE bug that
Jacob B. reported at [1] (which is a distinct problem, per Alexander's
analysis). v16 and v17 seem like a gray area, but we do know that
they have #19560. Even if we limit our ambition in those branches
to fixing that problem, I don't see a simple localized fix for it.

Thoughts?

regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CA%2BCOZaBVFHS-eXL7a53iZfNJyv5LqD5t1eSbYDavYxLSyfkL6A%40mail.gmail.com