From 3fb6055044cf6c2c0ad317ca386005b0c6c000a1 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
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 <nathandbossart@gmail.com>
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
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

