Perform join removal by editing the query's jointree. analyzejoins.c decided which joins could be dropped by consulting the planner's derived data structures, but then implemented the removal by updating those structures in-place. That is a lot of fiddly work, and nothing keeps it in step with the rest of the planner: remove_leftjoinrel_from_query only bothered to update "parts of the planner's data structures that will actually be consulted later", with no good way to know what those are. Bug #19560 is one consequence. In that report, removing a join leaves an EquivalenceClass that now gives rise to a base restriction clause, but base restriction clauses have already been generated and nothing reconsiders them, so the WHERE condition disappears from the plan and we return wrong answers.
The self-join elimination code has the same design and the same type of hazard. We have seen many related bugs over the years too, so it's time to do something drastic. To fix, do the removals by editing root->parse->jointree (which is a far simpler and more stable representation than the derived data), and then have query_planner() discard everything it computed from the jointree and derive it over again. This requires quite a bit less code, and doesn't require touching analyzejoins.c every time we change the data derived by query_planner(). For typical cases it can actually save a bit of planning time, though in cases where we have to iterate the derivation loop many times it does add some time. reduce_unique_semijoins() gets the same treatment: rather than deleting the semijoin's SpecialJoinInfo and relying on the jointree not being consulted again, it now changes the JoinExpr's jointype to JOIN_INNER and recalculates everything. Some plans change in the join regression test. Qual evaluation order shifts in a few cases, because the conditions now reach later planning in jointree order rather than in whatever order the removal code re-distributed them. A few plans improve, since the rebuilt relation targetlists no longer carry columns that only a removed join needed. We also detect a constant-false filter condition whose test used to carry a FIXME label. One plan gets marginally worse, because the old code recomputed attr_needed from equivalence classes after a join removal; that is more accurate than what deconstruct_jointree() derives from the original clauses, but we no longer do that. Making that recomputation happen anyway could be worth doing, but it should be considered independently and perhaps implemented differently. Back-patch to v16, on the grounds that the introduction of varnullingrels in v16 made the old approach significantly more complex and bug-prone; notably, bug #19560 does not manifest before v16. In released branches, do not remove externally-visible fixup functions such as remove_join_clause_from_rels, in case any extensions are relying on them; but they're no longer used by core code. But we must nonetheless break API/ABI for remove_useless_joins, reduce_unique_semijoins, and remove_useless_self_joins, as those now have different outputs and very different behavior than before. It seems unlikely that any extensions are calling those; but just in case, make the breakage more obvious by renaming remove_useless_joins to remove_useless_outer_joins, which is a more sensible name for it anyway since the addition of remove_useless_self_joins. Full disclosure: initial drafts of this patch were made with Claude Opus 4.8. Bug: #19560 Reported-by: Orestis Markou <[email protected]> Author: Tom Lane <[email protected]> Reviewed-by: Richard Guo <[email protected]> Reviewed-by: Thom Brown <[email protected]> Reviewed-by: Jacob Brazeal <[email protected]> Discussion: https://postgr.es/m/[email protected] Backpatch-through: 16 Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/986870baa06bc70245ee731d83486d4ed529c223 Modified Files -------------- src/backend/optimizer/plan/analyzejoins.c | 614 ++++++++++-------------------- src/backend/optimizer/plan/planmain.c | 92 ++++- src/backend/optimizer/plan/planner.c | 18 +- src/backend/rewrite/rewriteManip.c | 60 ++- src/include/nodes/primnodes.h | 4 + src/include/optimizer/planmain.h | 4 +- src/test/regress/expected/join.out | 32 ++ src/test/regress/expected/rowsecurity.out | 11 + src/test/regress/sql/join.sql | 25 ++ src/test/regress/sql/rowsecurity.sql | 4 + 10 files changed, 414 insertions(+), 450 deletions(-)
