Hi, On Thu, Aug 27, 2026 at 9:47 AM Nathan Bossart <[email protected]> wrote: > > On Thu, Aug 27, 2026 at 09:41:49AM -0700, Bharath Rupireddy wrote: > > REPACK (CONCURRENTLY) starts a decoding bgworker and then waits in > > start_repack_decoding_worker() for the worker to set a shared-memory > > flag. The wait has no liveness check on the worker itself. If the > > worker never reaches that point (e.g., fork failure under memory > > pressure, or when BecomeLockGroupMember() returns false, or early exit > > before the shm_mq error redirect is set up), the backend waits > > indefinitely with no way out other than cancellation. I reproduced > > this with an induced fork failure, so I think we need to tighten this > > for both PG19 and HEAD branches. > > Oops, I just concurrently reported this [0]. Note that teardown can > deadlock, too. > > [0] https://postgr.es/m/apBpOVZOyqrakEr_%40nathan
Thanks. Here's my first attempt at fixing both the fork failure hang and the teardown deadlock. I tried to use the parallel query approach as much as possible. The fork failure hang can occur because the backend running concurrent repack sleeps on a CV and ignores the SIGUSR1 sent via bgw_notify_pid by the postmaster upon fork failure. The teardown deadlock can occur because the backend waits for the worker to exit before detaching the error queue, while the worker is blocked writing to a full queue, so both end up waiting on each other. Although these issues seem rare to hit, I think it's good to tighten the repack code because users can see them via SQL. Therefore, I think we need to backpatch these to PG19. Please have a look at the attached patch. While here, I noticed that the same wait event is used for both the worker startup wait and the file export wait. Ideally these would have separate wait events, but given that the startup wait is typically very short, reusing the same one seems fine. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
From 3fb6055044cf6c2c0ad317ca386005b0c6c000a1 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Fri, 28 Aug 2026 19:22:57 +0000 Subject: [PATCH v1] Fix hang and deadlock in concurrent REPACK worker handling. REPACK CONCURRENTLY starts a background worker that decodes the changes made to a table while it is being repacked, and the backend running the command coordinates with that worker through shared memory and an error queue. If the worker fails to start, for example when fork() fails, the backend can wait for it forever. While waiting for the worker to finish its setup, the backend sleeps on a condition variable, which only wakes on its own signal. It therefore ignores the SIGUSR1 that the postmaster sends via bgw_notify_pid when the worker fails to start or exits, so the backend never notices and keeps waiting while holding its lock on the table. Fix this by waiting on the process latch instead, as parallel query does, and checking the worker's status on each wakeup; if the worker is gone before it finished setting up, report an error. The worker now sets the backend's latch once it is ready, so the normal case still wakes promptly. A second problem can occur when the backend stops the worker. It waits for the worker to exit before detaching from the error queue. If the worker is blocked writing into a full error queue, it waits for the backend to read from it, while the backend waits for the worker to exit, so neither makes progress. Fix this too by detaching from the error queue before waiting for the worker to exit, again following what parallel query does. The blocked write then fails and the worker can exit. These are unlikely to hit in practice, but a user can trigger them through SQL, so backpatch to 19, where REPACK CONCURRENTLY was introduced. Reported-by: Nathan Bossart <[email protected]> Reported-by: Bharath Rupireddy <[email protected]> Author: Bharath Rupireddy <[email protected]> Discussion: https://postgr.es/m/CALj2ACVAxA9HxvFe8HSspTJ-UO4Aoz%3DkuQdZBeLrod0gqUxH3g%40mail.gmail.com Discussion: https://postgr.es/m/apBpOVZOyqrakEr_@nathan Backpatch-through: 19 --- src/backend/commands/repack.c | 65 +++++++++++++++++++++++++--- src/backend/commands/repack_worker.c | 8 +++- 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 477c86b2ba6..8100238578a 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -3678,16 +3678,41 @@ start_repack_decoding_worker(Oid relid) errmsg("out of background worker slots"), errhint("You might need to increase \"%s\".", "max_worker_processes")); + /* + * Associate the worker's handle with the error queue, just as if it had + * been passed to shm_mq_attach(); we passed NULL there because the worker + * did not exist yet. This lets ProcessRepackMessages() notice the worker + * is gone instead of blocking on the queue. parallel.c binds its error + * queues the same way. + */ + shm_mq_set_handle(decoding_worker->error_mqh, decoding_worker->handle); + /* * The decoding setup must be done before the caller can have XID assigned * for any reason, otherwise the worker might end up in a deadlock, * waiting for the caller's transaction to end. Therefore wait here until * the worker indicates that it has the logical decoding initialized. + * + * We wait on our latch, not on the condition variable. The worker sets + * our latch once it is initialized, and the postmaster sends us SIGUSR1 + * via bgw_notify_pid if the worker fails to start or exits, which sets + * our latch too. A condition variable would not do here: it returns only + * on its own signal, not on such a latch wakeup, so a worker that never + * starts (e.g. fork failure) would leave us waiting forever while holding + * ShareUpdateExclusiveLock on the table. */ - ConditionVariablePrepareToSleep(&shared->cv); for (;;) { bool initialized; + BgwHandleStatus status; + pid_t pid; + + /* + * Drain any messages from the worker first. This rethrows an error + * the worker reported (so we surface that rather than the generic + * failure below) and lets the wait be cancelled. + */ + CHECK_FOR_INTERRUPTS(); SpinLockAcquire(&shared->mutex); initialized = shared->initialized; @@ -3696,9 +3721,22 @@ start_repack_decoding_worker(Oid relid) if (initialized) break; - ConditionVariableSleep(&shared->cv, WAIT_EVENT_REPACK_WORKER_EXPORT); + /* Give up if the worker is gone before it got initialized. */ + status = GetBackgroundWorkerPid(decoding_worker->handle, &pid); + if (status == BGWH_STOPPED) + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("REPACK decoding worker failed to start"), + errhint("More details may be available in the server log.")); + if (status == BGWH_POSTMASTER_DIED) + ereport(FATAL, + errcode(ERRCODE_ADMIN_SHUTDOWN), + errmsg("postmaster exited during REPACK command")); + + (void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, -1, + WAIT_EVENT_REPACK_WORKER_EXPORT); + ResetLatch(MyLatch); } - ConditionVariableCancelSleep(); } /* @@ -3714,6 +3752,19 @@ stop_repack_decoding_worker(void) if (decoding_worker == NULL) return; + /* + * Detach from the error queue before waiting for the worker to exit. + * Otherwise a worker blocked writing into a full queue would wait for us + * to read from it while we wait for the worker to exit, and neither would + * make progress. Detaching lets the worker's write fail so that it can + * exit. DestroyParallelContext() detaches first for the same reason. + */ + if (decoding_worker->error_mqh != NULL) + { + shm_mq_detach(decoding_worker->error_mqh); + decoding_worker->error_mqh = NULL; + } + /* Terminate the worker process, if one is running. */ if (decoding_worker->handle != NULL) { @@ -3742,8 +3793,6 @@ stop_repack_decoding_worker(void) * critical because the CV lives in the DSM that we're about to detach, so * if we omit it, later automatic cleanup tries to clear freed memory. */ - if (decoding_worker->error_mqh != NULL) - shm_mq_detach(decoding_worker->error_mqh); ConditionVariableCancelSleep(); if (decoding_worker->seg != NULL) dsm_detach(decoding_worker->seg); @@ -3851,9 +3900,11 @@ ProcessRepackMessages(void) /* * Nothing to do if we haven't launched the worker yet or have already - * terminated it. + * terminated it. stop_repack_decoding_worker() detaches the error queue + * before clearing decoding_worker, so also bail out once error_mqh is + * gone. */ - if (decoding_worker == NULL) + if (decoding_worker == NULL || decoding_worker->error_mqh == NULL) return; /* diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c index af7e2a94764..c498cc86b2e 100644 --- a/src/backend/commands/repack_worker.c +++ b/src/backend/commands/repack_worker.c @@ -129,11 +129,15 @@ RepackWorkerMain(Datum main_arg) */ decoding_ctx = repack_setup_logical_decoding(shared->relid); - /* Announce that we're ready. */ + /* + * Announce that we're ready. The backend waits for this on its latch (see + * start_repack_decoding_worker()), so set it rather than signal the + * condition variable. + */ SpinLockAcquire(&shared->mutex); shared->initialized = true; SpinLockRelease(&shared->mutex); - ConditionVariableSignal(&shared->cv); + SetLatch(&shared->backend_proc->procLatch); /* There doesn't seem to a nice API to set these */ XactIsoLevel = XACT_REPEATABLE_READ; -- 2.47.3
