REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start

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]/messages/by-id/apBpOVZOyqrakEr_@nathan. Note that teardown can
deadlock, too.

[0]: /messages/by-id/apBpOVZOyqrakEr_@nathan

--
nathan

Hi,

On Thu, Aug 27, 2026 at 9:47 AM Nathan Bossart <nathandbossart@gmail.com> 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] /messages/by-id/apBpOVZOyqrakEr_@nathan

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

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, Aug 27, 2026 at 9:47 AM Nathan Bossart <nathandbossart@gmail.com> 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] /messages/by-id/apBpOVZOyqrakEr_@nathan

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.

Thanks for the fix(es). One thing I'm not sure I understand is:

@@ -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;

/*

I don't think that stop_repack_decoding_worker() can clear ->error_mqh w/o
also clearing decoding_worker.

Other than that, I'm not sure you need to mention the condition variable in
the comments. And maybe even the mentions of parallel workers are not
necessary.

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.

I think that initially I also considered this situation not worth a new wait
event, but I probably had missed an existing one:
WAIT_EVENT_BGWORKER_STARTUP. It's already used for multiple workers, so we
could perhaps use it here.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

Hi,

On Mon, Aug 31, 2026 at 3:31 AM Antonin Houska <ah@cybertec.at> wrote:

Thanks for the fix(es). One thing I'm not sure I understand is:

Thanks for reviewing.

@@ -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;

/*

I don't think that stop_repack_decoding_worker() can clear ->error_mqh w/o
also clearing decoding_worker.

With the patch, there's a window where stop_repack_decoding_worker()
sets the error queue pointer to NULL before setting the worker's
shared memory pointer to NULL. I would like to keep this check.|

Other than that, I'm not sure you need to mention the condition variable in
the comments. And maybe even the mentions of parallel workers are not
necessary.

Reworded the comments.

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.

I think that initially I also considered this situation not worth a new wait
event, but I probably had missed an existing one:
WAIT_EVENT_BGWORKER_STARTUP. It's already used for multiple workers, so we
could perhaps use it here.

I think using WAIT_EVENT_BGWORKER_STARTUP is fine, because if a worker
is ever stuck here, one can look at pg_stat_activity to tell whether
this is the REPACK decoding worker. I've done that in the 0002 patch.

Please find the attached v2 patches.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

On Mon, Aug 31, 2026 at 11:14 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

On Mon, Aug 31, 2026 at 3:31 AM Antonin Houska <ah@cybertec.at> wrote:

Thanks for the fix(es). One thing I'm not sure I understand is:

Thanks for reviewing.

@@ -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;

/*

I don't think that stop_repack_decoding_worker() can clear ->error_mqh w/o
also clearing decoding_worker.

With the patch, there's a window where stop_repack_decoding_worker()
sets the error queue pointer to NULL before setting the worker's
shared memory pointer to NULL. I would like to keep this check.|

Other than that, I'm not sure you need to mention the condition variable in
the comments. And maybe even the mentions of parallel workers are not
necessary.

Reworded the comments.

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.

I think that initially I also considered this situation not worth a new wait
event, but I probably had missed an existing one:
WAIT_EVENT_BGWORKER_STARTUP. It's already used for multiple workers, so we
could perhaps use it here.

I think using WAIT_EVENT_BGWORKER_STARTUP is fine, because if a worker
is ever stuck here, one can look at pg_stat_activity to tell whether
this is the REPACK decoding worker. I've done that in the 0002 patch.

Please find the attached v2 patches.

I've reviewed the patch and I have two questions:

+   /*
+    * 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 worke
+    * 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 second sentence starting with "This lets ..." is unclear to me.
ProcessRepackMessage() receives the message with nowait, no?

---
+       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)

parallel.c handles BGWH_STOPPED differently; it checks that the worker
stopped without attaching to the error queue. IIUC if the worker
stopped after attaching to the error queue, an error message should
arrive at the leader and the leader could handle it in the next CFI.
Is there any reason why start_repack_decoding_worker() handles it
differently?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Masahiko Sawada <sawada.mshk@gmail.com> wrote:

+   /*
+    * 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 worke
+    * 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 second sentence starting with "This lets ..." is unclear to me.
ProcessRepackMessage() receives the message with nowait, no?

Although it probably does not break anything if we pass the handle to
shm_mq_set_handle() unnecessarily, I'm thinking if it's necessary for an error
queue.

AFAIU the purpose of the handle is to realize that it makes no sense to wait
for the background worker to attach to the queue. Once the worker has
attached, the worker's handle is no longer needed to detect that the worker
detached: the already-attached worker itself is supposed to call
shm_mq_detach().

If I use the parallel worker for reference, I think the leader should not be
asked (via the PROCSIG_PARALLEL_MESSAGE signal) to read from the error queue
until the worker has attached and redirected its messages to the queue - see
in ParallelWorkerMain():

mqh = shm_mq_attach(mq, seg, NULL);
pq_redirect_to_shm_mq(seg, mqh);

Therefore, the leader should not need the worker's handle.

I also thought about the other source of the PROCSIG_PARALLEL_MESSAGE signal,
a few lines above the call to shm_mq_attach():

before_shmem_exit(ParallelWorkerShutdown, PointerGetDatum(seg));

If the worker called proc_exit() after this, but before the call of
shm_mq_attach(), the worker handle might be useful for the leader to conclude
that the worker hasn't detached yet. However, the coding of
ProcessParallelMessages() is such that shm_mq_receive() can return
SHM_MQ_WOULD_BLOCK in such a case, so the ERROR "lost connection to parallel
worker" is not raised anyway.

While thinking about that ERROR, I realize that I might have misunderstood its
purpose, and therefore it's not present in
ProcessRepackMessage(). Nevertheless, I'm still not sure I understand when
exactly should that ERROR be raised. Per commit 2badb5afb8, the function
ParallelWorkerShutdown() is related. However, AFAICS, when the worker code
calls proc_exit(), the ParallelWorkerShutdown() callback can send the
PROCSIG_PARALLEL_MESSAGE signal while the worker is still attached to the
error queue. In that case, the ERROR is not raised.

I'm probably missing something. Thanks in advance for any hint.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

Hi,

On Fri, Sep 4, 2026 at 5:11 PM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

I've reviewed the patch and I have two questions:

Thanks for reviewing it.

+   /*
+    * 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

worke

+ * 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 second sentence starting with "This lets ..." is unclear to me.
ProcessRepackMessage() receives the message with nowait, no?

Yes, the read is nowait, and I get that the comment is misleading. All we
do here is connect the backend running concurrent repack (the receiver)
with the decoding worker (the sender) by saving the worker's handle to the
error message queue. We could not pass that handle to shm_mq_attach() above
because the sender did not exist at that point. The handle only matters
until the sender attaches to the queue. Until then, the receiver uses the
handle to check whether the sender is still alive, and reports detached if
the sender is already gone, instead of blocking on the queue. parallel.c
does the same for its workers.

---
+       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)

parallel.c handles BGWH_STOPPED differently; it checks that the worker
stopped without attaching to the error queue. IIUC if the worker
stopped after attaching to the error queue, an error message should
arrive at the leader and the leader could handle it in the next CFI.
Is there any reason why start_repack_decoding_worker() handles it
differently?

Right. There are two cases. The worker started and exited after attaching
to the error message queue, or it started and exited before attaching to
it. In the second case, throwing the generic error is correct, and in the
first the next CFI captures the worker's error. The attached v3 handles
both. Please have a look.

After thinking more about this, I simplified the handling. There are two
things the backend needs to wait for. First, for the worker to come up and
attach to the error message queue. Second, for the worker to set up the
logical decoding machinery before it waits for snapshot export. The first
wait catches fork failures and worker startup issues. I used similar logic
to what parallel.c uses for this. The second wait catches failures that
happen after the worker starts up and attaches to the error queue but
before it finishes setting up the logical decoding machinery. I kept the
shared memory initialized flag with the CV wait as-is for this.

Although the initialized flag wait may seem redundant with the snapshot
export wait in get_initial_snapshot(), I would still keep it because it
ensures the worker has fully set up the decoding before the backend
proceeds.

Dividing this into two separate waits (waiting for the worker to come up
and attach to the error message queue, and then waiting for it to finish
setup) makes the logic simpler, lets us reuse most of parallel.c's code, is
easier to reason about, and fixes the hang issue without letting the
backend reach the snapshot export wait with the worker not fully ready.

Please find the attached v3 patch.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Sep 4, 2026 at 5:11 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

After thinking more about this, I simplified the handling. There are two things the backend needs to wait for. First, for the worker to come up
and attach to the error message queue. Second, for the worker to set up the logical decoding machinery before it waits for snapshot export.
The first wait catches fork failures and worker startup issues. I used similar logic to what parallel.c uses for this. The second wait catches
failures that happen after the worker starts up and attaches to the error queue but before it finishes setting up the logical decoding
machinery. I kept the shared memory initialized flag with the CV wait as-is for this.

Although the initialized flag wait may seem redundant with the snapshot export wait in get_initial_snapshot(), I would still keep it because it
ensures the worker has fully set up the decoding before the backend proceeds.

Dividing this into two separate waits (waiting for the worker to come up and attach to the error message queue, and then waiting for it to
finish setup) makes the logic simpler, lets us reuse most of parallel.c's code, is easier to reason about, and fixes the hang issue without letting
the backend reach the snapshot export wait with the worker not fully ready.

Another reason for two separate waits is that two separate event types make
sense: WAIT_EVENT_BGWORKER_STARTUP and WAIT_EVENT_REPACK_WORKER_EXPORT.

Please find the attached v3 patch.

Just two comments:

* wait_for_repack_worker_to_attach() - as the decoding_worker variable is
declared static, this function does not necessarily need the argument.

* Regarding comment: when the following is reached, the error message queue
has already been detached, so no implicit detaching should happen. Also,
there are no "other shared memory queues".

+       /*
+        * If we have allocated a shared memory segment, detach it. This will
+        * implicitly detach the error message queue, and any other shared memory
+        * queues, stored there.
+        */
+       if (decoding_worker->seg != NULL)
+       {
+               dsm_detach(decoding_worker->seg);
+               decoding_worker->seg = NULL;
+       }

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

Hi,

On Tue, Sep 8, 2026 at 12:17 AM Antonin Houska <ah@cybertec.at> wrote:

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Sep 4, 2026 at 5:11 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

After thinking more about this, I simplified the handling. There are two things the backend needs to wait for. First, for the worker to come up
and attach to the error message queue. Second, for the worker to set up the logical decoding machinery before it waits for snapshot export.
The first wait catches fork failures and worker startup issues. I used similar logic to what parallel.c uses for this. The second wait catches
failures that happen after the worker starts up and attaches to the error queue but before it finishes setting up the logical decoding
machinery. I kept the shared memory initialized flag with the CV wait as-is for this.

Although the initialized flag wait may seem redundant with the snapshot export wait in get_initial_snapshot(), I would still keep it because it
ensures the worker has fully set up the decoding before the backend proceeds.

Dividing this into two separate waits (waiting for the worker to come up and attach to the error message queue, and then waiting for it to
finish setup) makes the logic simpler, lets us reuse most of parallel.c's code, is easier to reason about, and fixes the hang issue without letting
the backend reach the snapshot export wait with the worker not fully ready.

Another reason for two separate waits is that two separate event types make
sense: WAIT_EVENT_BGWORKER_STARTUP and WAIT_EVENT_REPACK_WORKER_EXPORT.

That's correct.

Please find the attached v3 patch.

Just two comments:

Thanks for reviewing it.

* wait_for_repack_worker_to_attach() - as the decoding_worker variable is
declared static, this function does not necessarily need the argument.

Removed the function parameter.

* Regarding comment: when the following is reached, the error message queue
has already been detached, so no implicit detaching should happen. Also,
there are no "other shared memory queues".

+       /*
+        * If we have allocated a shared memory segment, detach it. This will
+        * implicitly detach the error message queue, and any other shared memory
+        * queues, stored there.
+        */
+       if (decoding_worker->seg != NULL)
+       {
+               dsm_detach(decoding_worker->seg);
+               decoding_worker->seg = NULL;
+       }

I borrowed it from parallel.c. I agree it can be simplified, and I
have done that.

Please find the attached v4 patch.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com