pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Simplify executor's determination of whether to use parallelism. 6+ messages / 1 participants [nested] [flat]
* pgsql: Simplify executor's determination of whether to use parallelism. @ 2024-12-09 19:38 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-12-09 19:38 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Simplify executor's determination of whether to use parallelism. Our parallel-mode code only works when we are executing a query in full, so ExecutePlan must disable parallel mode when it is asked to do partial execution. The previous logic for this involved passing down a flag (variously named execute_once or run_once) from callers of ExecutorRun or PortalRun. This is overcomplicated, and unsurprisingly some of the callers didn't get it right, since it requires keeping state that not all of them have handy; not to mention that the requirements for it were undocumented. That led to assertion failures in some corner cases. The only state we really need for this is the existing QueryDesc.already_executed flag, so let's just put all the responsibility in ExecutePlan. (It could have been done in ExecutorRun too, leading to a slightly shorter patch -- but if there's ever more than one caller of ExecutePlan, it seems better to have this logic in the subroutine than the callers.) This makes those ExecutorRun/PortalRun parameters unnecessary. In master it seems okay to just remove them, returning the API for those functions to what it was before parallelism. Such an API break is clearly not okay in stable branches, but for them we can just leave the parameters in place after documenting that they do nothing. Per report from Yugo Nagata, who also reviewed and tested this patch. Back-patch to all supported branches. Discussion: https://postgr.es/m/20241206062549.710dc01cf91224809dd6c0e1@sraoss.co.jp Branch ------ REL_17_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/556f7b7bc18d34ddec45392965c3b3038206bb62 Modified Files -------------- src/backend/executor/execMain.c | 47 ++++++++++++++++++----------------------- src/backend/tcop/postgres.c | 4 ++-- src/backend/tcop/pquery.c | 13 ++++-------- src/include/executor/execdesc.h | 2 +- src/include/utils/portal.h | 2 +- 5 files changed, 29 insertions(+), 39 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Simplify executor's determination of whether to use parallelism. @ 2024-12-09 19:38 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-12-09 19:38 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Simplify executor's determination of whether to use parallelism. Our parallel-mode code only works when we are executing a query in full, so ExecutePlan must disable parallel mode when it is asked to do partial execution. The previous logic for this involved passing down a flag (variously named execute_once or run_once) from callers of ExecutorRun or PortalRun. This is overcomplicated, and unsurprisingly some of the callers didn't get it right, since it requires keeping state that not all of them have handy; not to mention that the requirements for it were undocumented. That led to assertion failures in some corner cases. The only state we really need for this is the existing QueryDesc.already_executed flag, so let's just put all the responsibility in ExecutePlan. (It could have been done in ExecutorRun too, leading to a slightly shorter patch -- but if there's ever more than one caller of ExecutePlan, it seems better to have this logic in the subroutine than the callers.) This makes those ExecutorRun/PortalRun parameters unnecessary. In master it seems okay to just remove them, returning the API for those functions to what it was before parallelism. Such an API break is clearly not okay in stable branches, but for them we can just leave the parameters in place after documenting that they do nothing. Per report from Yugo Nagata, who also reviewed and tested this patch. Back-patch to all supported branches. Discussion: https://postgr.es/m/20241206062549.710dc01cf91224809dd6c0e1@sraoss.co.jp Branch ------ REL_13_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/4d20bad5e9c6afc389d037f6a2cb3c627e845dd2 Modified Files -------------- src/backend/executor/execMain.c | 47 ++++++++++++++++++----------------------- src/backend/tcop/postgres.c | 4 ++-- src/backend/tcop/pquery.c | 13 ++++-------- src/include/executor/execdesc.h | 2 +- src/include/utils/portal.h | 2 +- 5 files changed, 29 insertions(+), 39 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Simplify executor's determination of whether to use parallelism. @ 2024-12-09 19:38 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-12-09 19:38 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Simplify executor's determination of whether to use parallelism. Our parallel-mode code only works when we are executing a query in full, so ExecutePlan must disable parallel mode when it is asked to do partial execution. The previous logic for this involved passing down a flag (variously named execute_once or run_once) from callers of ExecutorRun or PortalRun. This is overcomplicated, and unsurprisingly some of the callers didn't get it right, since it requires keeping state that not all of them have handy; not to mention that the requirements for it were undocumented. That led to assertion failures in some corner cases. The only state we really need for this is the existing QueryDesc.already_executed flag, so let's just put all the responsibility in ExecutePlan. (It could have been done in ExecutorRun too, leading to a slightly shorter patch -- but if there's ever more than one caller of ExecutePlan, it seems better to have this logic in the subroutine than the callers.) This makes those ExecutorRun/PortalRun parameters unnecessary. In master it seems okay to just remove them, returning the API for those functions to what it was before parallelism. Such an API break is clearly not okay in stable branches, but for them we can just leave the parameters in place after documenting that they do nothing. Per report from Yugo Nagata, who also reviewed and tested this patch. Back-patch to all supported branches. Discussion: https://postgr.es/m/20241206062549.710dc01cf91224809dd6c0e1@sraoss.co.jp Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/3eea7a0c97e94f9570af87317ce3f6a41eb62768 Modified Files -------------- contrib/auto_explain/auto_explain.c | 8 ++-- contrib/pg_stat_statements/pg_stat_statements.c | 9 ++--- src/backend/commands/copyto.c | 2 +- src/backend/commands/createas.c | 2 +- src/backend/commands/explain.c | 2 +- src/backend/commands/extension.c | 2 +- src/backend/commands/matview.c | 2 +- src/backend/commands/portalcmds.c | 2 +- src/backend/commands/prepare.c | 2 +- src/backend/executor/execMain.c | 53 ++++++++++--------------- src/backend/executor/execParallel.c | 3 +- src/backend/executor/functions.c | 2 +- src/backend/executor/spi.c | 2 +- src/backend/tcop/postgres.c | 2 - src/backend/tcop/pquery.c | 17 ++------ src/include/executor/execdesc.h | 2 +- src/include/executor/executor.h | 7 ++-- src/include/tcop/pquery.h | 2 +- src/include/utils/portal.h | 1 - 19 files changed, 49 insertions(+), 73 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Simplify executor's determination of whether to use parallelism. @ 2024-12-09 19:38 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-12-09 19:38 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Simplify executor's determination of whether to use parallelism. Our parallel-mode code only works when we are executing a query in full, so ExecutePlan must disable parallel mode when it is asked to do partial execution. The previous logic for this involved passing down a flag (variously named execute_once or run_once) from callers of ExecutorRun or PortalRun. This is overcomplicated, and unsurprisingly some of the callers didn't get it right, since it requires keeping state that not all of them have handy; not to mention that the requirements for it were undocumented. That led to assertion failures in some corner cases. The only state we really need for this is the existing QueryDesc.already_executed flag, so let's just put all the responsibility in ExecutePlan. (It could have been done in ExecutorRun too, leading to a slightly shorter patch -- but if there's ever more than one caller of ExecutePlan, it seems better to have this logic in the subroutine than the callers.) This makes those ExecutorRun/PortalRun parameters unnecessary. In master it seems okay to just remove them, returning the API for those functions to what it was before parallelism. Such an API break is clearly not okay in stable branches, but for them we can just leave the parameters in place after documenting that they do nothing. Per report from Yugo Nagata, who also reviewed and tested this patch. Back-patch to all supported branches. Discussion: https://postgr.es/m/20241206062549.710dc01cf91224809dd6c0e1@sraoss.co.jp Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/8a95ad3b205cfce4441bdb6cae0475e1304cc3a9 Modified Files -------------- src/backend/executor/execMain.c | 47 ++++++++++++++++++----------------------- src/backend/tcop/postgres.c | 4 ++-- src/backend/tcop/pquery.c | 13 ++++-------- src/include/executor/execdesc.h | 2 +- src/include/utils/portal.h | 2 +- 5 files changed, 29 insertions(+), 39 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Simplify executor's determination of whether to use parallelism. @ 2024-12-09 19:38 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-12-09 19:38 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Simplify executor's determination of whether to use parallelism. Our parallel-mode code only works when we are executing a query in full, so ExecutePlan must disable parallel mode when it is asked to do partial execution. The previous logic for this involved passing down a flag (variously named execute_once or run_once) from callers of ExecutorRun or PortalRun. This is overcomplicated, and unsurprisingly some of the callers didn't get it right, since it requires keeping state that not all of them have handy; not to mention that the requirements for it were undocumented. That led to assertion failures in some corner cases. The only state we really need for this is the existing QueryDesc.already_executed flag, so let's just put all the responsibility in ExecutePlan. (It could have been done in ExecutorRun too, leading to a slightly shorter patch -- but if there's ever more than one caller of ExecutePlan, it seems better to have this logic in the subroutine than the callers.) This makes those ExecutorRun/PortalRun parameters unnecessary. In master it seems okay to just remove them, returning the API for those functions to what it was before parallelism. Such an API break is clearly not okay in stable branches, but for them we can just leave the parameters in place after documenting that they do nothing. Per report from Yugo Nagata, who also reviewed and tested this patch. Back-patch to all supported branches. Discussion: https://postgr.es/m/20241206062549.710dc01cf91224809dd6c0e1@sraoss.co.jp Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/4089b9bd6f19362ccb093d7e1d55282a6577acb4 Modified Files -------------- src/backend/executor/execMain.c | 47 ++++++++++++++++++----------------------- src/backend/tcop/postgres.c | 4 ++-- src/backend/tcop/pquery.c | 13 ++++-------- src/include/executor/execdesc.h | 2 +- src/include/utils/portal.h | 2 +- 5 files changed, 29 insertions(+), 39 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Simplify executor's determination of whether to use parallelism. @ 2024-12-09 19:38 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-12-09 19:38 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Simplify executor's determination of whether to use parallelism. Our parallel-mode code only works when we are executing a query in full, so ExecutePlan must disable parallel mode when it is asked to do partial execution. The previous logic for this involved passing down a flag (variously named execute_once or run_once) from callers of ExecutorRun or PortalRun. This is overcomplicated, and unsurprisingly some of the callers didn't get it right, since it requires keeping state that not all of them have handy; not to mention that the requirements for it were undocumented. That led to assertion failures in some corner cases. The only state we really need for this is the existing QueryDesc.already_executed flag, so let's just put all the responsibility in ExecutePlan. (It could have been done in ExecutorRun too, leading to a slightly shorter patch -- but if there's ever more than one caller of ExecutePlan, it seems better to have this logic in the subroutine than the callers.) This makes those ExecutorRun/PortalRun parameters unnecessary. In master it seems okay to just remove them, returning the API for those functions to what it was before parallelism. Such an API break is clearly not okay in stable branches, but for them we can just leave the parameters in place after documenting that they do nothing. Per report from Yugo Nagata, who also reviewed and tested this patch. Back-patch to all supported branches. Discussion: https://postgr.es/m/20241206062549.710dc01cf91224809dd6c0e1@sraoss.co.jp Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/bb649b553c589a3fd1f36b2280e984622e6fdcdb Modified Files -------------- src/backend/executor/execMain.c | 47 ++++++++++++++++++----------------------- src/backend/tcop/postgres.c | 4 ++-- src/backend/tcop/pquery.c | 13 ++++-------- src/include/executor/execdesc.h | 2 +- src/include/utils/portal.h | 2 +- 5 files changed, 29 insertions(+), 39 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-12-09 19:38 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-12-09 19:38 pgsql: Simplify executor's determination of whether to use parallelism. Tom Lane <tgl@sss.pgh.pa.us> 2024-12-09 19:38 pgsql: Simplify executor's determination of whether to use parallelism. Tom Lane <tgl@sss.pgh.pa.us> 2024-12-09 19:38 pgsql: Simplify executor's determination of whether to use parallelism. Tom Lane <tgl@sss.pgh.pa.us> 2024-12-09 19:38 pgsql: Simplify executor's determination of whether to use parallelism. Tom Lane <tgl@sss.pgh.pa.us> 2024-12-09 19:38 pgsql: Simplify executor's determination of whether to use parallelism. Tom Lane <tgl@sss.pgh.pa.us> 2024-12-09 19:38 pgsql: Simplify executor's determination of whether to use parallelism. 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