pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. 6+ messages / 1 participants [nested] [flat]
* pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. @ 2023-07-14 15:46 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2023-07-14 15:46 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Account for optimized MinMax aggregates during SS_finalize_plan. We are capable of optimizing MIN() and MAX() aggregates on indexed columns into subqueries that exploit the index, rather than the normal thing of scanning the whole table. When we do this, we replace the Aggref node(s) with Params referencing subquery outputs. Such Params really ought to be included in the per-plan-node extParam/allParam sets computed by SS_finalize_plan. However, we've never done so up to now because of an ancient implementation choice to perform that substitution during set_plan_references, which runs after SS_finalize_plan, so that SS_finalize_plan never sees these Params. This seems like clearly a bug, yet there have been no field reports of problems that could trace to it. This may be because the types of Plan nodes that could contain Aggrefs do not have any of the rescan optimizations that are controlled by extParam/allParam. Nonetheless it seems certain to bite us someday, so let's fix it in a self-contained patch that can be back-patched if we find a case in which there's a live bug pre-v17. The cleanest fix would be to perform a separate tree walk to do these substitutions before SS_finalize_plan runs. That seems unattractive, first because a whole-tree mutation pass is expensive, and second because we lack infrastructure for visiting expression subtrees in a Plan tree, so that we'd need a new function knowing as much as SS_finalize_plan knows about that. I also considered swapping the order of SS_finalize_plan and set_plan_references, but that fell foul of various assumptions that seem tricky to fix. So the approach adopted here is to teach SS_finalize_plan itself to check for such Aggrefs. I refactored things a bit in setrefs.c to avoid having three copies of the code that does that. Given the lack of any currently-known bug, no test case here. Discussion: https://postgr.es/m/2391880.1689025003@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/d0d44049d1262aed2eee906d26af852948206db0 Modified Files -------------- src/backend/optimizer/plan/setrefs.c | 68 +++++++++++++++++++++------------- src/backend/optimizer/plan/subselect.c | 25 +++++++++++-- src/include/optimizer/planmain.h | 2 + 3 files changed, 66 insertions(+), 29 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. @ 2024-05-18 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-05-18 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Account for optimized MinMax aggregates during SS_finalize_plan. We are capable of optimizing MIN() and MAX() aggregates on indexed columns into subqueries that exploit the index, rather than the normal thing of scanning the whole table. When we do this, we replace the Aggref node(s) with Params referencing subquery outputs. Such Params really ought to be included in the per-plan-node extParam/allParam sets computed by SS_finalize_plan. However, we've never done so up to now because of an ancient implementation choice to perform that substitution during set_plan_references, which runs after SS_finalize_plan, so that SS_finalize_plan never sees these Params. The cleanest fix would be to perform a separate tree walk to do these substitutions before SS_finalize_plan runs. That seems unattractive, first because a whole-tree mutation pass is expensive, and second because we lack infrastructure for visiting expression subtrees in a Plan tree, so that we'd need a new function knowing as much as SS_finalize_plan knows about that. I also considered swapping the order of SS_finalize_plan and set_plan_references, but that fell foul of various assumptions that seem tricky to fix. So the approach adopted here is to teach SS_finalize_plan itself to check for such Aggrefs. I refactored things a bit in setrefs.c to avoid having three copies of the code that does that. Back-patch of v17 commits d0d44049d and 779ac2c74. When d0d44049d went in, there was no evidence that it was fixing a reachable bug, so I refrained from back-patching. Now we have such evidence. Per bug #18465 from Hal Takahara. Back-patch to all supported branches. Discussion: https://postgr.es/m/18465-2fae927718976b22@postgresql.org Discussion: https://postgr.es/m/2391880.1689025003@sss.pgh.pa.us Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/ce0d1654463e4f6080cfe83df74ed978945608a1 Modified Files -------------- src/backend/optimizer/plan/setrefs.c | 68 ++++++++++++++++++++------------ src/backend/optimizer/plan/subselect.c | 25 ++++++++++-- src/include/optimizer/planmain.h | 2 + src/test/regress/expected/aggregates.out | 31 +++++++++++++++ src/test/regress/sql/aggregates.sql | 10 +++++ 5 files changed, 107 insertions(+), 29 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. @ 2024-05-18 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-05-18 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Account for optimized MinMax aggregates during SS_finalize_plan. We are capable of optimizing MIN() and MAX() aggregates on indexed columns into subqueries that exploit the index, rather than the normal thing of scanning the whole table. When we do this, we replace the Aggref node(s) with Params referencing subquery outputs. Such Params really ought to be included in the per-plan-node extParam/allParam sets computed by SS_finalize_plan. However, we've never done so up to now because of an ancient implementation choice to perform that substitution during set_plan_references, which runs after SS_finalize_plan, so that SS_finalize_plan never sees these Params. The cleanest fix would be to perform a separate tree walk to do these substitutions before SS_finalize_plan runs. That seems unattractive, first because a whole-tree mutation pass is expensive, and second because we lack infrastructure for visiting expression subtrees in a Plan tree, so that we'd need a new function knowing as much as SS_finalize_plan knows about that. I also considered swapping the order of SS_finalize_plan and set_plan_references, but that fell foul of various assumptions that seem tricky to fix. So the approach adopted here is to teach SS_finalize_plan itself to check for such Aggrefs. I refactored things a bit in setrefs.c to avoid having three copies of the code that does that. Back-patch of v17 commits d0d44049d and 779ac2c74. When d0d44049d went in, there was no evidence that it was fixing a reachable bug, so I refrained from back-patching. Now we have such evidence. Per bug #18465 from Hal Takahara. Back-patch to all supported branches. Discussion: https://postgr.es/m/18465-2fae927718976b22@postgresql.org Discussion: https://postgr.es/m/2391880.1689025003@sss.pgh.pa.us Branch ------ REL_12_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/686c995fc2223426706ee3f2472e220e7aac4041 Modified Files -------------- src/backend/optimizer/plan/setrefs.c | 68 ++++++++++++++++++++------------ src/backend/optimizer/plan/subselect.c | 25 ++++++++++-- src/include/optimizer/planmain.h | 2 + src/test/regress/expected/aggregates.out | 31 +++++++++++++++ src/test/regress/sql/aggregates.sql | 10 +++++ 5 files changed, 107 insertions(+), 29 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. @ 2024-05-18 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-05-18 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Account for optimized MinMax aggregates during SS_finalize_plan. We are capable of optimizing MIN() and MAX() aggregates on indexed columns into subqueries that exploit the index, rather than the normal thing of scanning the whole table. When we do this, we replace the Aggref node(s) with Params referencing subquery outputs. Such Params really ought to be included in the per-plan-node extParam/allParam sets computed by SS_finalize_plan. However, we've never done so up to now because of an ancient implementation choice to perform that substitution during set_plan_references, which runs after SS_finalize_plan, so that SS_finalize_plan never sees these Params. The cleanest fix would be to perform a separate tree walk to do these substitutions before SS_finalize_plan runs. That seems unattractive, first because a whole-tree mutation pass is expensive, and second because we lack infrastructure for visiting expression subtrees in a Plan tree, so that we'd need a new function knowing as much as SS_finalize_plan knows about that. I also considered swapping the order of SS_finalize_plan and set_plan_references, but that fell foul of various assumptions that seem tricky to fix. So the approach adopted here is to teach SS_finalize_plan itself to check for such Aggrefs. I refactored things a bit in setrefs.c to avoid having three copies of the code that does that. Back-patch of v17 commits d0d44049d and 779ac2c74. When d0d44049d went in, there was no evidence that it was fixing a reachable bug, so I refrained from back-patching. Now we have such evidence. Per bug #18465 from Hal Takahara. Back-patch to all supported branches. Discussion: https://postgr.es/m/18465-2fae927718976b22@postgresql.org Discussion: https://postgr.es/m/2391880.1689025003@sss.pgh.pa.us Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/5ac340602e7481198f2576567665ce6facc03a89 Modified Files -------------- src/backend/optimizer/plan/setrefs.c | 68 ++++++++++++++++++++------------ src/backend/optimizer/plan/subselect.c | 25 ++++++++++-- src/include/optimizer/planmain.h | 2 + src/test/regress/expected/aggregates.out | 31 +++++++++++++++ src/test/regress/sql/aggregates.sql | 10 +++++ 5 files changed, 107 insertions(+), 29 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. @ 2024-05-18 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-05-18 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Account for optimized MinMax aggregates during SS_finalize_plan. We are capable of optimizing MIN() and MAX() aggregates on indexed columns into subqueries that exploit the index, rather than the normal thing of scanning the whole table. When we do this, we replace the Aggref node(s) with Params referencing subquery outputs. Such Params really ought to be included in the per-plan-node extParam/allParam sets computed by SS_finalize_plan. However, we've never done so up to now because of an ancient implementation choice to perform that substitution during set_plan_references, which runs after SS_finalize_plan, so that SS_finalize_plan never sees these Params. The cleanest fix would be to perform a separate tree walk to do these substitutions before SS_finalize_plan runs. That seems unattractive, first because a whole-tree mutation pass is expensive, and second because we lack infrastructure for visiting expression subtrees in a Plan tree, so that we'd need a new function knowing as much as SS_finalize_plan knows about that. I also considered swapping the order of SS_finalize_plan and set_plan_references, but that fell foul of various assumptions that seem tricky to fix. So the approach adopted here is to teach SS_finalize_plan itself to check for such Aggrefs. I refactored things a bit in setrefs.c to avoid having three copies of the code that does that. Back-patch of v17 commits d0d44049d and 779ac2c74. When d0d44049d went in, there was no evidence that it was fixing a reachable bug, so I refrained from back-patching. Now we have such evidence. Per bug #18465 from Hal Takahara. Back-patch to all supported branches. Discussion: https://postgr.es/m/18465-2fae927718976b22@postgresql.org Discussion: https://postgr.es/m/2391880.1689025003@sss.pgh.pa.us Branch ------ REL_13_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/7f90a5dc368620e097fac8690b8698e75f8eccba Modified Files -------------- src/backend/optimizer/plan/setrefs.c | 68 ++++++++++++++++++++------------ src/backend/optimizer/plan/subselect.c | 25 ++++++++++-- src/include/optimizer/planmain.h | 2 + src/test/regress/expected/aggregates.out | 31 +++++++++++++++ src/test/regress/sql/aggregates.sql | 10 +++++ 5 files changed, 107 insertions(+), 29 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. @ 2024-05-18 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-05-18 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Account for optimized MinMax aggregates during SS_finalize_plan. We are capable of optimizing MIN() and MAX() aggregates on indexed columns into subqueries that exploit the index, rather than the normal thing of scanning the whole table. When we do this, we replace the Aggref node(s) with Params referencing subquery outputs. Such Params really ought to be included in the per-plan-node extParam/allParam sets computed by SS_finalize_plan. However, we've never done so up to now because of an ancient implementation choice to perform that substitution during set_plan_references, which runs after SS_finalize_plan, so that SS_finalize_plan never sees these Params. The cleanest fix would be to perform a separate tree walk to do these substitutions before SS_finalize_plan runs. That seems unattractive, first because a whole-tree mutation pass is expensive, and second because we lack infrastructure for visiting expression subtrees in a Plan tree, so that we'd need a new function knowing as much as SS_finalize_plan knows about that. I also considered swapping the order of SS_finalize_plan and set_plan_references, but that fell foul of various assumptions that seem tricky to fix. So the approach adopted here is to teach SS_finalize_plan itself to check for such Aggrefs. I refactored things a bit in setrefs.c to avoid having three copies of the code that does that. Back-patch of v17 commits d0d44049d and 779ac2c74. When d0d44049d went in, there was no evidence that it was fixing a reachable bug, so I refrained from back-patching. Now we have such evidence. Per bug #18465 from Hal Takahara. Back-patch to all supported branches. Discussion: https://postgr.es/m/18465-2fae927718976b22@postgresql.org Discussion: https://postgr.es/m/2391880.1689025003@sss.pgh.pa.us Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/4ac385adc5b7ae6474fc8e8fb418131cf2e63b4b Modified Files -------------- src/backend/optimizer/plan/setrefs.c | 68 ++++++++++++++++++++------------ src/backend/optimizer/plan/subselect.c | 25 ++++++++++-- src/include/optimizer/planmain.h | 2 + src/test/regress/expected/aggregates.out | 31 +++++++++++++++ src/test/regress/sql/aggregates.sql | 10 +++++ 5 files changed, 107 insertions(+), 29 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-05-18 18:31 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-07-14 15:46 pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. Tom Lane <tgl@sss.pgh.pa.us> 2024-05-18 18:31 pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. Tom Lane <tgl@sss.pgh.pa.us> 2024-05-18 18:31 pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. Tom Lane <tgl@sss.pgh.pa.us> 2024-05-18 18:31 pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. Tom Lane <tgl@sss.pgh.pa.us> 2024-05-18 18:31 pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. Tom Lane <tgl@sss.pgh.pa.us> 2024-05-18 18:31 pgsql: Account for optimized MinMax aggregates during SS_finalize_plan. 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