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 11:46:36 -0500
Message-ID: <3694355.1731084396@sss.pgh.pa.us> (raw)
In-Reply-To: <20241107193823.dc.nmisch@google.com>
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>
Noah Misch <noah@leadboat.com> writes:
> On Thu, Nov 07, 2024 at 02:29:19PM -0500, Tom Lane wrote:
>> Ah, that could be a way out. Stick an INTERRUPTS_CAN_BE_PROCESSED()
>> call somewhere in there?
> Exactly. If !INTERRUPTS_CAN_BE_PROCESSED(), proceed as though no workers can
> be launched.
>> That could even allow us to revert the
>> planner change, which would simplify testing of the executor change.
> True.
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.
We could argue about whether or not to revert the planner change.
But I'd prefer to do so, because as things stand it creates a
hard-to-reason-about source of plan instability.
regards, tom lane
Attachments:better-fix-for-noninterruptible-lockup.patch (text/x-diff, 2.1K) downloaddiff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.cindex 4a2e352d57..0c86543d40 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.
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: <3694355.1731084396@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