public inbox for [email protected]
help / color / mirror / Atom feedRe: SIGQUIT handling, redux
4+ messages / 3 participants
[nested] [flat]
* Re: SIGQUIT handling, redux
@ 2023-08-02 16:25 Andres Freund <[email protected]>
2023-08-02 16:35 ` Re: SIGQUIT handling, redux Tom Lane <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Andres Freund @ 2023-08-02 16:25 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]
Hi,
On 2020-09-11 11:52:55 -0400, Tom Lane wrote:
> It's simple enough that maybe we could back-patch it, once it's
> aged awhile in HEAD. OTOH, given the lack of field reports of
> trouble here, I'm not sure back-patching is worth the risk.
FWIW, looking at collected stack traces in azure, there's a slow but steady
stream of crashes below StartupPacketTimeoutHandler. Most seem to be things
like
libcrypto->malloc->StartupPacketTimeoutHandler->proc_exit->socket_close->free->crash
there's a few other variants, some where the stack apparently was not
decipherable for the relevant tooling.
Note that this wouldn't even include cases where this caused hangs - which is
quite common IME.
Unsurprisingly just in versions before 14, where this change went in.
I think that might be enough evidence for backpatching the commit? I've not
heard of issues due to the checks in check_on_shmem_exit_lists_are_empty().
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: SIGQUIT handling, redux
2023-08-02 16:25 Re: SIGQUIT handling, redux Andres Freund <[email protected]>
@ 2023-08-02 16:35 ` Tom Lane <[email protected]>
2023-08-02 16:48 ` Re: SIGQUIT handling, redux Andres Freund <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Tom Lane @ 2023-08-02 16:35 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: [email protected]
Andres Freund <[email protected]> writes:
> On 2020-09-11 11:52:55 -0400, Tom Lane wrote:
>> It's simple enough that maybe we could back-patch it, once it's
>> aged awhile in HEAD. OTOH, given the lack of field reports of
>> trouble here, I'm not sure back-patching is worth the risk.
> FWIW, looking at collected stack traces in azure, there's a slow but steady
> stream of crashes below StartupPacketTimeoutHandler. ...
> Unsurprisingly just in versions before 14, where this change went in.
> I think that might be enough evidence for backpatching the commit? I've not
> heard of issues due to the checks in check_on_shmem_exit_lists_are_empty().
I'd be willing to take a look at this in a few weeks when $real_life
is a bit less demanding. Right before minor releases would likely be
a bad idea anyway.
regards, tom lane
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: SIGQUIT handling, redux
2023-08-02 16:25 Re: SIGQUIT handling, redux Andres Freund <[email protected]>
2023-08-02 16:35 ` Re: SIGQUIT handling, redux Tom Lane <[email protected]>
@ 2023-08-02 16:48 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Andres Freund @ 2023-08-02 16:48 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]
Hi,
On 2023-08-02 12:35:19 -0400, Tom Lane wrote:
> Andres Freund <[email protected]> writes:
> > On 2020-09-11 11:52:55 -0400, Tom Lane wrote:
> >> It's simple enough that maybe we could back-patch it, once it's
> >> aged awhile in HEAD. OTOH, given the lack of field reports of
> >> trouble here, I'm not sure back-patching is worth the risk.
>
> > FWIW, looking at collected stack traces in azure, there's a slow but steady
> > stream of crashes below StartupPacketTimeoutHandler. ...
> > Unsurprisingly just in versions before 14, where this change went in.
> > I think that might be enough evidence for backpatching the commit? I've not
> > heard of issues due to the checks in check_on_shmem_exit_lists_are_empty().
>
> I'd be willing to take a look at this in a few weeks when $real_life
> is a bit less demanding.
Cool.
> Right before minor releases would likely be a bad idea anyway.
Agreed. I had just waded through the stacks, so I thought it'd be worth
bringing up, didn't intend to imply we should backpatch immediately.
Aside: Analyzing this kind of thing at scale is made considerably more painful
by "expected" ereport(PANIC)s (say out of disk space during WAL writes)
calling abort() and dumping core... While there are other PANICs you really
want cores of.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH v1 5/8] convert Sharedsort->{currentWorker,workersFinished} to atomics
@ 2026-07-09 20:04 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Nathan Bossart @ 2026-07-09 20:04 UTC (permalink / raw)
---
src/backend/utils/sort/tuplesort.c | 30 ++++++++----------------------
1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index c0e7527b9ca..81e0b2816d6 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -104,6 +104,7 @@
#include "commands/tablespace.h"
#include "miscadmin.h"
#include "pg_trace.h"
+#include "port/atomics.h"
#include "port/pg_bitutils.h"
#include "storage/shmem.h"
#include "utils/guc.h"
@@ -340,9 +341,6 @@ struct Tuplesortstate
*/
struct Sharedsort
{
- /* mutex protects all fields prior to tapes */
- slock_t mutex;
-
/*
* currentWorker generates ordinal identifier numbers for parallel sort
* workers. These start from 0, and are always gapless.
@@ -351,8 +349,8 @@ struct Sharedsort
* is equal to state.nParticipants within the leader, leader is ready to
* merge worker runs.
*/
- int currentWorker;
- int workersFinished;
+ pg_atomic_uint32 currentWorker;
+ pg_atomic_uint32 workersFinished;
/* Temporary file space */
SharedFileSet fileset;
@@ -3252,9 +3250,8 @@ tuplesort_initialize_shared(Sharedsort *shared, int nWorkers, dsm_segment *seg)
Assert(nWorkers > 0);
- SpinLockInit(&shared->mutex);
- shared->currentWorker = 0;
- shared->workersFinished = 0;
+ pg_atomic_init_u32(&shared->currentWorker, 0);
+ pg_atomic_init_u32(&shared->workersFinished, 0);
SharedFileSetInit(&shared->fileset, seg);
shared->nTapes = nWorkers;
for (i = 0; i < nWorkers; i++)
@@ -3291,16 +3288,9 @@ tuplesort_attach_shared(Sharedsort *shared, dsm_segment *seg)
static int
worker_get_identifier(Tuplesortstate *state)
{
- Sharedsort *shared = state->shared;
- int worker;
-
Assert(WORKER(state));
- SpinLockAcquire(&shared->mutex);
- worker = shared->currentWorker++;
- SpinLockRelease(&shared->mutex);
-
- return worker;
+ return pg_atomic_fetch_add_u32(&state->shared->currentWorker, 1);
}
/*
@@ -3342,10 +3332,8 @@ worker_freeze_result_tape(Tuplesortstate *state)
LogicalTapeFreeze(state->result_tape, &output);
/* Store properties of output tape, and update finished worker count */
- SpinLockAcquire(&shared->mutex);
shared->tapes[state->worker] = output;
- shared->workersFinished++;
- SpinLockRelease(&shared->mutex);
+ pg_atomic_fetch_add_u32(&shared->workersFinished, 1);
}
/*
@@ -3387,9 +3375,7 @@ leader_takeover_tapes(Tuplesortstate *state)
Assert(LEADER(state));
Assert(nParticipants >= 1);
- SpinLockAcquire(&shared->mutex);
- workersFinished = shared->workersFinished;
- SpinLockRelease(&shared->mutex);
+ workersFinished = pg_atomic_read_membarrier_u32(&shared->workersFinished);
if (nParticipants != workersFinished)
elog(ERROR, "cannot take over tapes before all workers finish");
--
2.50.1 (Apple Git-155)
--Ot5x3ffkWEuxilWO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename=v1-0006-convert-SharedFileSet-refcnt-to-an-atomic.patch
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-07-09 20:04 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-08-02 16:25 Re: SIGQUIT handling, redux Andres Freund <[email protected]>
2023-08-02 16:35 ` Tom Lane <[email protected]>
2023-08-02 16:48 ` Andres Freund <[email protected]>
2026-07-09 20:04 [PATCH v1 5/8] convert Sharedsort->{currentWorker,workersFinished} to atomics Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox