From: Tom Lane <tgl@sss.pgh.pa.us>
To: Noah Misch <noah@leadboat.com>
Cc: Francesco Degrassi <francesco.degrassi@optionfactory.net>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: Leader backend hang on IPC/ParallelFinish when LWLock held at parallel query start
Date: Fri, 08 Nov 2024 12:56:55 -0500
Message-ID: <3793541.1731088615@sss.pgh.pa.us> (raw)
In-Reply-To: <3694355.1731084396@sss.pgh.pa.us>
References: <CAC-SaSzHUKT=vZJ8MPxYdC_URPfax+yoA1hKTcF4ROz_Q6z0_Q@mail.gmail.com>
<20240918030159.2a.nmisch@google.com>
<1737570.1726633422@sss.pgh.pa.us>
<20240918050434.13.nmisch@google.com>
<3441950.1730999841@sss.pgh.pa.us>
<20241107192233.94.nmisch@google.com>
<3459938.1731007759@sss.pgh.pa.us>
<20241107193823.dc.nmisch@google.com>
<3694355.1731084396@sss.pgh.pa.us>
I wrote:
> Here's a proposed patch along that line. I left the test case from
> ac04aa84a alone, since it works perfectly well to test this way too.
I'd modeled that on the existing recovery code for DSM segment creation
failure, just below. But a look at the code coverage report shows
(unsurprisingly) that that path is never exercised in our regression
tests, so I wondered if it actually works ... and it doesn't work
very well. To test, I lobotomized InitializeParallelDSM to always
force pcxt->nworkers = 0. That results in a bunch of unsurprising
regression test diffs, plus a couple of
+ERROR: could not find key 4 in shm TOC at 0x229f138
which turns out to be the fault of ExecHashJoinReInitializeDSM:
it's not accounting for the possibility that we didn't really
start a parallel hash join.
I'm also not happy about ReinitializeParallelWorkers'
Assert(pcxt->nworkers >= nworkers_to_launch);
The one existing caller manages not to trigger that because it's
careful to reduce its request based on pcxt->nworkers, but it
doesn't seem to me that callers should be expected to have to.
So I end with the attached. There might still be some more issues
that the regression tests don't reach, but I think this is the
best we can do for today.
regards, tom lane
Attachments:v2-better-fix-for-noninterruptible-lockup.patch (text/x-diff, 3.7K) downloaddiff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.cindex 4a2e352d57..a10bf02ccf 100644--- a/src/backend/access/transam/parallel.c+++ b/src/backend/access/transam/parallel.c@@ -230,6 +230,15 @@ InitializeParallelDSM(ParallelContext *pcxt)
shm_toc_estimate_chunk(&pcxt->estimator, sizeof(FixedParallelState));
shm_toc_estimate_keys(&pcxt->estimator, 1);
+ /*+ * If we manage to reach here while non-interruptible, it's unsafe to+ * launch any workers: we would fail to process interrupts sent by them.+ * We can deal with that edge case by pretending no workers were+ * requested.+ */+ if (!INTERRUPTS_CAN_BE_PROCESSED())+ pcxt->nworkers = 0;+
/*
* Normally, the user will have requested at least one worker process, but
* if by chance they have not, we can skip a bunch of things here.
@@ -476,6 +485,9 @@ InitializeParallelDSM(ParallelContext *pcxt)
shm_toc_insert(pcxt->toc, PARALLEL_KEY_ENTRYPOINT, entrypointstate);
}
+ /* Update nworkers_to_launch, in case we changed nworkers above. */+ pcxt->nworkers_to_launch = pcxt->nworkers;+
/* Restore previous memory context. */
MemoryContextSwitchTo(oldcontext);
}
@@ -539,10 +551,11 @@ ReinitializeParallelWorkers(ParallelContext *pcxt, int nworkers_to_launch)
{
/*
* The number of workers that need to be launched must be less than the
- * number of workers with which the parallel context is initialized.+ * number of workers with which the parallel context is initialized. But+ * the caller might not know that InitializeParallelDSM reduced nworkers,+ * so just silently trim the request.
*/
- Assert(pcxt->nworkers >= nworkers_to_launch);- pcxt->nworkers_to_launch = nworkers_to_launch;+ pcxt->nworkers_to_launch = Min(pcxt->nworkers, nworkers_to_launch);
}
/*
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.cindex 2f7170604d..6c3009fba0 100644--- a/src/backend/executor/nodeHashjoin.c+++ b/src/backend/executor/nodeHashjoin.c@@ -1713,8 +1713,13 @@ void
ExecHashJoinReInitializeDSM(HashJoinState *state, ParallelContext *pcxt)
{
int plan_node_id = state->js.ps.plan->plan_node_id;
- ParallelHashJoinState *pstate =- shm_toc_lookup(pcxt->toc, plan_node_id, false);+ ParallelHashJoinState *pstate;++ /* Nothing to do if we failed to create a DSM segment. */+ if (pcxt->seg == NULL)+ return;++ pstate = shm_toc_lookup(pcxt->toc, plan_node_id, false);
/*
* It would be possible to reuse the shared hash table in single-batch
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.cindex 3e3f0d486a..1f78dc3d53 100644--- a/src/backend/optimizer/plan/planner.c+++ b/src/backend/optimizer/plan/planner.c@@ -342,11 +342,6 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
* we want to allow parallel inserts in general; updates and deletes have
* additional problems especially around combo CIDs.)
*
- * We don't try to use parallel mode unless interruptible. The leader- * expects ProcessInterrupts() calls to reach HandleParallelMessages().- * Even if we called HandleParallelMessages() another way, starting a- * parallel worker is too delay-prone to be prudent when uncancellable.- *
* For now, we don't try to use parallel mode if we're running inside a
* parallel worker. We might eventually be able to relax this
* restriction, but for now it seems best not to have parallel workers
@@ -357,7 +352,6 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
parse->commandType == CMD_SELECT &&
!parse->hasModifyingCTE &&
max_parallel_workers_per_gather > 0 &&
- INTERRUPTS_CAN_BE_PROCESSED() &&
!IsParallelWorker())
{
/* all the cheap tests pass, so scan the query tree */
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, noah@leadboat.com, francesco.degrassi@optionfactory.net, pgsql-bugs@lists.postgresql.org
Subject: Re: Leader backend hang on IPC/ParallelFinish when LWLock held at parallel query start
In-Reply-To: <3793541.1731088615@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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