From f759d3921640b109a2cc283810dbccff59724914 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Mon, 31 Aug 2026 17:24:14 +0000 Subject: [PATCH v2 1/2] 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 Reported-by: Bharath Rupireddy Author: Bharath Rupireddy Reviewed-by: Antonin Houska 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 | 62 ++++++++++++++++++++++++---- src/backend/commands/repack_worker.c | 8 +++- 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 477c86b2ba6..02b8f3b07ec 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -3678,16 +3678,38 @@ 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. + */ + 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. The worker sets it 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. That way a worker that + * never starts (e.g. fork failure) does not 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 +3718,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 +3749,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. + */ + 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 +3790,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 +3897,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