pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Perform join removal by editing the query's jointree. 5+ messages / 1 participants [nested] [flat]
* pgsql: Perform join removal by editing the query's jointree. @ 2026-08-28 19:13 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2026-08-28 19:13 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org 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 <orestis@orestis.gr> Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Thom Brown <thom@linux.com> Reviewed-by: Jacob Brazeal <jacob.brazeal@gmail.com> Discussion: https://postgr.es/m/1186816.1784573544@sss.pgh.pa.us Backpatch-through: 16 Branch ------ REL_18_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/9f25197bf27c4c4a02d754842bc4055d83be735b Modified Files -------------- src/backend/optimizer/plan/analyzejoins.c | 1840 ++++++++++------------------- src/backend/optimizer/plan/planmain.c | 98 +- src/backend/optimizer/plan/planner.c | 18 +- src/backend/rewrite/rewriteManip.c | 49 +- src/include/nodes/primnodes.h | 4 + src/include/optimizer/planmain.h | 6 +- src/test/regress/expected/join.out | 156 ++- src/test/regress/expected/rowsecurity.out | 11 + src/test/regress/sql/join.sql | 58 +- src/test/regress/sql/rowsecurity.sql | 4 + 10 files changed, 972 insertions(+), 1272 deletions(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
* pgsql: Perform join removal by editing the query's jointree. @ 2026-08-28 19:13 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2026-08-28 19:13 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org 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 <orestis@orestis.gr> Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Thom Brown <thom@linux.com> Reviewed-by: Jacob Brazeal <jacob.brazeal@gmail.com> Discussion: https://postgr.es/m/1186816.1784573544@sss.pgh.pa.us 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(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
* pgsql: Perform join removal by editing the query's jointree. @ 2026-08-28 19:13 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2026-08-28 19:13 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org 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 <orestis@orestis.gr> Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Thom Brown <thom@linux.com> Reviewed-by: Jacob Brazeal <jacob.brazeal@gmail.com> Discussion: https://postgr.es/m/1186816.1784573544@sss.pgh.pa.us Backpatch-through: 16 Branch ------ REL_17_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/13466d1f78394a8ec2d6d0b1c9d8bf5ac3acb99a Modified Files -------------- src/backend/optimizer/plan/analyzejoins.c | 628 ++++++++++-------------------- src/backend/optimizer/plan/planmain.c | 92 ++++- src/backend/optimizer/plan/planner.c | 18 +- src/backend/rewrite/rewriteManip.c | 63 ++- 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, 417 insertions(+), 464 deletions(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
* pgsql: Perform join removal by editing the query's jointree. @ 2026-08-28 19:13 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2026-08-28 19:13 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org 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 <orestis@orestis.gr> Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Thom Brown <thom@linux.com> Reviewed-by: Jacob Brazeal <jacob.brazeal@gmail.com> Discussion: https://postgr.es/m/1186816.1784573544@sss.pgh.pa.us Backpatch-through: 16 Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/2ebf25e7d70a8fce31ace78d723fa9271ab8af72 Modified Files -------------- src/backend/optimizer/path/equivclass.c | 51 +- src/backend/optimizer/plan/analyzejoins.c | 1878 ++++++++++------------------- src/backend/optimizer/plan/initsplan.c | 185 +-- src/backend/optimizer/plan/planmain.c | 98 +- src/backend/optimizer/plan/planner.c | 18 +- src/backend/optimizer/util/joininfo.c | 36 - src/backend/optimizer/util/placeholder.c | 27 - src/backend/rewrite/rewriteManip.c | 109 +- src/include/nodes/primnodes.h | 4 + src/include/optimizer/joininfo.h | 3 - src/include/optimizer/paths.h | 2 - src/include/optimizer/placeholder.h | 1 - src/include/optimizer/planmain.h | 14 +- src/include/rewrite/rewriteManip.h | 19 - src/test/regress/expected/join.out | 159 ++- src/test/regress/expected/rowsecurity.out | 11 + src/test/regress/sql/join.sql | 58 +- src/test/regress/sql/rowsecurity.sql | 4 + src/tools/pgindent/typedefs.list | 1 - 19 files changed, 1005 insertions(+), 1673 deletions(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
* pgsql: Perform join removal by editing the query's jointree. @ 2026-08-28 19:13 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2026-08-28 19:13 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org 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 <orestis@orestis.gr> Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Thom Brown <thom@linux.com> Reviewed-by: Jacob Brazeal <jacob.brazeal@gmail.com> Discussion: https://postgr.es/m/1186816.1784573544@sss.pgh.pa.us Backpatch-through: 16 Branch ------ REL_19_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/0ab90a5c94188ab0a2113e36c73f093f741129c1 Modified Files -------------- src/backend/optimizer/path/equivclass.c | 51 +- src/backend/optimizer/plan/analyzejoins.c | 1878 ++++++++++------------------- src/backend/optimizer/plan/initsplan.c | 185 +-- src/backend/optimizer/plan/planmain.c | 98 +- src/backend/optimizer/plan/planner.c | 18 +- src/backend/optimizer/util/joininfo.c | 36 - src/backend/optimizer/util/placeholder.c | 27 - src/backend/rewrite/rewriteManip.c | 109 +- src/include/nodes/primnodes.h | 4 + src/include/optimizer/joininfo.h | 3 - src/include/optimizer/paths.h | 2 - src/include/optimizer/placeholder.h | 1 - src/include/optimizer/planmain.h | 14 +- src/include/rewrite/rewriteManip.h | 19 - src/test/regress/expected/join.out | 159 ++- src/test/regress/expected/rowsecurity.out | 11 + src/test/regress/sql/join.sql | 58 +- src/test/regress/sql/rowsecurity.sql | 4 + src/tools/pgindent/typedefs.list | 1 - 19 files changed, 1005 insertions(+), 1673 deletions(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-08-28 19:13 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-08-28 19:13 pgsql: Perform join removal by editing the query's jointree. Tom Lane <tgl@sss.pgh.pa.us> 2026-08-28 19:13 pgsql: Perform join removal by editing the query's jointree. Tom Lane <tgl@sss.pgh.pa.us> 2026-08-28 19:13 pgsql: Perform join removal by editing the query's jointree. Tom Lane <tgl@sss.pgh.pa.us> 2026-08-28 19:13 pgsql: Perform join removal by editing the query's jointree. Tom Lane <tgl@sss.pgh.pa.us> 2026-08-28 19:13 pgsql: Perform join removal by editing the query's jointree. Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox