NotificationsYou must be signed in to change notification settings
Fork4.9k
Star17.7k

Commit6e39ca6

committed

Improve fix for not entering parallel mode when holding interrupts.

Commitac04aa8 put the shutoff for this into the planner, which isnot ideal because it doesn't prevent us from re-using a previouslymade parallel plan. Revert the planner change and instead put theshutoff into InitializeParallelDSM, modeling it on the existing codethere for recovering from failure to allocate a DSM segment.However, that code path is mostly untested, and testing a bit hardershowed there's at least one bug: ExecHashJoinReInitializeDSM is notprepared for us to have skipped doing parallel DSM setup. I alsothought the Assert in ReinitializeParallelWorkers is prettyill-advised, and replaced it with a silent Min() operation.The existing test case added byac04aa8 serves fine to test thisversion of the fix, so no change needed there.Patch by me, but thanks to Noah Misch for the core idea that wecould shut off worker creation when !INTERRUPTS_CAN_BE_PROCESSED.Back-patch to v12, asac04aa8 was.Discussion:https://postgr.es/m/CAC-SaSzHUKT=vZJ8MPxYdC_URPfax+yoA1hKTcF4ROz_Q6z0_Q@mail.gmail.com

1 parent9c47574 commit6e39ca6Copy full SHA for 6e39ca6

File tree

3 files changed

+16

-8

lines changed

src/backend
- access/transam
  - parallel.c
- executor
  - nodeHashjoin.c
- optimizer/plan
  - planner.c

3 files changed

+16

-8

lines changed

`‎src/backend/access/transam/parallel.c`

Lines changed: 9 additions & 0 deletions

Original file line number	Diff line number	Diff line change
`@@ -219,6 +219,15 @@ InitializeParallelDSM(ParallelContext *pcxt)`
`219`	`219`	`shm_toc_estimate_chunk(&pcxt->estimator,sizeof(FixedParallelState));`
`220`	`220`	`shm_toc_estimate_keys(&pcxt->estimator,1);`
`221`	`221`
	`222`	`+/*`
	`223`	`+ * If we manage to reach here while non-interruptible, it's unsafe to`
	`224`	`+ * launch any workers: we would fail to process interrupts sent by them.`
	`225`	`+ * We can deal with that edge case by pretending no workers were`
	`226`	`+ * requested.`
	`227`	`+ */`
	`228`	`+if (!INTERRUPTS_CAN_BE_PROCESSED())`
	`229`	`+pcxt->nworkers=0;`
	`230`	`+`
`222`	`231`	`/*`
`223`	`232`	`* Normally, the user will have requested at least one worker process, but`
`224`	`233`	`* if by chance they have not, we can skip a bunch of things here.`

`‎src/backend/executor/nodeHashjoin.c`

Lines changed: 7 additions & 2 deletions

Original file line number	Diff line number	Diff line change
`@@ -1516,8 +1516,13 @@ void`
`1516`	`1516`	`ExecHashJoinReInitializeDSM(HashJoinStatestate,ParallelContextcxt)`
`1517`	`1517`	`{`
`1518`	`1518`	`intplan_node_id=state->js.ps.plan->plan_node_id;`
`1519`		`-ParallelHashJoinState*pstate=`
`1520`		`-shm_toc_lookup(cxt->toc,plan_node_id, false);`
	`1519`	`+ParallelHashJoinState*pstate;`
	`1520`	`+`
	`1521`	`+/* Nothing to do if we failed to create a DSM segment. */`
	`1522`	`+if (cxt->seg==NULL)`
	`1523`	`+return;`
	`1524`	`+`
	`1525`	`+pstate=shm_toc_lookup(cxt->toc,plan_node_id, false);`
`1521`	`1526`
`1522`	`1527`	`/*`
`1523`	`1528`	`* It would be possible to reuse the shared hash table in single-batch`

`‎src/backend/optimizer/plan/planner.c`

Lines changed: 0 additions & 6 deletions

Original file line number	Diff line number	Diff line change
`@@ -331,11 +331,6 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)`
`331`	`331`	`* general; updates and deletes have additional problems especially around`
`332`	`332`	`* combo CIDs.)`
`333`	`333`	`*`
`334`		`- * We don't try to use parallel mode unless interruptible. The leader`
`335`		`- * expects ProcessInterrupts() calls to reach HandleParallelMessages().`
`336`		`- * Even if we called HandleParallelMessages() another way, starting a`
`337`		`- * parallel worker is too delay-prone to be prudent when uncancellable.`
`338`		`- *`
`339`	`334`	`* For now, we don't try to use parallel mode if we're running inside a`
`340`	`335`	`* parallel worker. We might eventually be able to relax this`
`341`	`336`	`* restriction, but for now it seems best not to have parallel workers`
`@@ -346,7 +341,6 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)`
`346`	`341`	`parse->commandType==CMD_SELECT&&`
`347`	`342`	`!parse->hasModifyingCTE&&`
`348`	`343`	`max_parallel_workers_per_gather>0&&`
`349`		`-INTERRUPTS_CAN_BE_PROCESSED()&&`
`350`	`344`	`!IsParallelWorker())`
`351`	`345`	`{`
`352`	`346`	`/* all the cheap tests pass, so scan the query tree */`

0 commit comments

Comments

(0)

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Commit6e39ca6

File tree

3 files changed

3 files changed

`‎src/backend/access/transam/parallel.c`

`‎src/backend/executor/nodeHashjoin.c`

`‎src/backend/optimizer/plan/planner.c`

0 commit comments