pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedREPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
18+ messages / 7 participants
[nested] [flat]
* REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-08-27 16:41 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2026-08-27 16:41 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Álvaro Herrera <alvherre@kurilemu.de>
Hi,
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.
Fix would be to check
GetBackgroundWorkerPid()/WaitForBackgroundWorkerStartup() and error
out when the worker has not started. If okay, I can send a patch.
Thoughts?
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-08-27 16:47 Nathan Bossart <nathandbossart@gmail.com>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Nathan Bossart @ 2026-08-27 16:47 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: pgsql-hackers; Álvaro Herrera <alvherre@kurilemu.de>
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
--
nathan
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-08-28 20:20 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2026-08-28 20:20 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers; Álvaro Herrera <alvherre@kurilemu.de>
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] 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
Attachments:
[application/x-patch] v1-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch (7.5K, ../../CALj2ACUpxBdkAfvfT4=DsQ4k5Faz7W9Sqk27j+Y4HWexPSvUdA@mail.gmail.com/2-v1-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-08-31 10:31 Antonin Houska <ah@cybertec.at>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Antonin Houska @ 2026-08-31 10:31 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
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] 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.
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-08-31 18:14 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2026-08-31 18:14 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
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
Attachments:
[application/octet-stream] v2-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch (7.3K, ../../CALj2ACVoM4cd5bhOhqP+qqaetL9gWvChCFEoym6GZHHbz-faJw@mail.gmail.com/2-v2-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch)
download | inline diff:
From f759d3921640b109a2cc283810dbccff59724914 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
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 <nathandbossart@gmail.com>
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Antonin Houska <ah@cybertec.at>
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
[application/octet-stream] v2-0002-Fix-repack-decoding-worker-startup-wait-event.patch (1.4K, ../../CALj2ACVoM4cd5bhOhqP+qqaetL9gWvChCFEoym6GZHHbz-faJw@mail.gmail.com/3-v2-0002-Fix-repack-decoding-worker-startup-wait-event.patch)
download | inline diff:
From aa586e6e18d4ecf999f593eb6983aa3b94892758 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 31 Aug 2026 18:08:41 +0000
Subject: [PATCH v2 2/2] Fix repack decoding worker startup wait event.
The backend running REPACK CONCURRENTLY waits for its decoding
worker to initialize logical decoding, but reported that wait as
REPACK_WORKER_EXPORT, which describes waiting for an output file
and is misleading here. Report it as the existing
BGWORKER_STARTUP event instead, as parallel query and others do,
so pg_stat_activity shows what the backend is really waiting for
without adding a new wait event.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Discussion: https://postgr.es/m/CALj2ACVAxA9HxvFe8HSspTJ-UO4Aoz%3DkuQdZBeLrod0gqUxH3g%40mail.gmail.com
Backpatch-through: 19
---
src/backend/commands/repack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 02b8f3b07ec..046bcafd067 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -3731,7 +3731,7 @@ start_repack_decoding_worker(Oid relid)
errmsg("postmaster exited during REPACK command"));
(void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, -1,
- WAIT_EVENT_REPACK_WORKER_EXPORT);
+ WAIT_EVENT_BGWORKER_STARTUP);
ResetLatch(MyLatch);
}
}
--
2.47.3
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-05 00:11 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 2 replies; 18+ messages in thread
From: Masahiko Sawada @ 2026-09-05 00:11 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-07 18:46 Antonin Houska <ah@cybertec.at>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
1 sibling, 0 replies; 18+ messages in thread
From: Antonin Houska @ 2026-09-07 18:46 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-07 19:15 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
1 sibling, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2026-09-07 19:15 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; Álvaro Herrera <alvherre@kurilemu.de>
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
Attachments:
[application/x-patch] v3-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch (10.4K, ../../CALj2ACVxb8rWTie8xBdXUHhaQosomHjQHRUqr2VX+Fx0qFXTiw@mail.gmail.com/3-v3-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch)
download | inline diff:
From acac900fe6ab2b56b989f9243b9673b618bde1f5 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 7 Sep 2026 12:14:16 +0000
Subject: [PATCH v3] 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 message 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 CV, 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 for the worker to attach to the error message
queue before waiting for it to initialize decoding. The backend
waits on its process latch and checks the worker's status on each
wakeup. If the worker stopped without attaching, the backend
reports a startup failure. If it attached but reported an error
before exiting, that error is surfaced through the error message
queue. Only after the worker has started does the backend wait on
the CV for decoding to be initialized, so a worker that never
starts no longer causes a hang.
A second problem can occur when the backend stops the worker. It
waits for the worker to exit before detaching from the error
message queue. If the worker is blocked writing into a full
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 terminating the worker and detaching from the
error message queue before waiting for the worker to exit.
Detaching before the wait lets the blocked write fail so that the
worker can exit. On a failed worker registration the backend now
also forgets the error message queue, so teardown does not try to
terminate a worker that was never started.
Both fixes closely follow parallel.c, which coordinates its
workers through an error message queue in the same way.
These are less likely to hit in practice, but a user can trigger
these through SQL, so backpatch to 19, where REPACK CONCURRENTLY
was introduced.
While here, report the startup wait as BGWORKER_STARTUP instead
of REPACK_WORKER_EXPORT, which describes waiting for an output
file and is misleading for a worker that has not started up yet.
Reported-by: Nathan Bossart <nathandbossart@gmail.com>
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Reviewed-by: Masahiko Sawada <sawada.mshk@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 | 170 +++++++++++++++++++++++++++++-----
1 file changed, 149 insertions(+), 21 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 477c86b2ba6..eb8780e1582 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -216,6 +216,7 @@ static Oid determine_clustered_index(Relation rel, bool usingindex,
const char *indexname);
static void start_repack_decoding_worker(Oid relid);
+static void wait_for_repack_worker_to_attach(DecodingWorker *worker);
static void stop_repack_decoding_worker(void);
static void stop_repack_decoding_worker_cb(int code, Datum arg);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
@@ -3673,10 +3674,34 @@ start_repack_decoding_worker(Oid relid)
bgw.bgw_notify_pid = MyProcPid;
if (!RegisterDynamicBackgroundWorker(&bgw, &decoding_worker->handle))
+ {
+ /*
+ * We couldn't register the worker, so forget the error message queue
+ * we set up for it. Otherwise stopping the worker later would try to
+ * terminate a worker that was never registered.
+ */
+ decoding_worker->handle = NULL;
+ shm_mq_detach(decoding_worker->error_mqh);
+ decoding_worker->error_mqh = NULL;
+
ereport(ERROR,
errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
errmsg("out of background worker slots"),
errhint("You might need to increase \"%s\".", "max_worker_processes"));
+ }
+
+ /*
+ * Now that the worker is registered, connect the error message queue to
+ * it.
+ */
+ shm_mq_set_handle(decoding_worker->error_mqh, decoding_worker->handle);
+
+ /*
+ * Make sure the worker has started before we wait for it to initialize
+ * decoding below, so that the failure-to-start case does not hang
+ * forever.
+ */
+ wait_for_repack_worker_to_attach(decoding_worker);
/*
* The decoding setup must be done before the caller can have XID assigned
@@ -3701,6 +3726,89 @@ start_repack_decoding_worker(Oid relid)
ConditionVariableCancelSleep();
}
+/*
+ * Wait for the decoding worker to start up, and throw an error if it fails
+ * to do so.
+ *
+ * This is similar to WaitForParallelWorkersToAttach(). The only reliable way
+ * to tell a worker that failed to start (fork failure, or an exit before it
+ * attached) from one that is merely slow is to check whether it became the
+ * sender on the error message queue. If it stopped without attaching, nothing
+ * was queued and we report the generic failure ourselves. If it attached, any
+ * error it reported is in the queue and is thrown when we process pending
+ * messages, either here or later while we wait for it to initialize decoding.
+ */
+static void
+wait_for_repack_worker_to_attach(DecodingWorker *worker)
+{
+ bool worker_attached = false;
+
+ for (;;)
+ {
+ BgwHandleStatus status;
+ shm_mq *mq;
+ int rc;
+ pid_t pid;
+
+ /*
+ * This will process any repack messages that are pending and it may
+ * also throw an error propagated from a worker.
+ */
+ CHECK_FOR_INTERRUPTS();
+
+ /* If the worker is known to have attached, we're done. */
+ if (worker_attached)
+ break;
+
+ /* If error_mqh is NULL, the worker has already exited cleanly. */
+ if (worker->error_mqh == NULL)
+ {
+ worker_attached = true;
+ continue;
+ }
+
+ status = GetBackgroundWorkerPid(worker->handle, &pid);
+ if (status == BGWH_STARTED)
+ {
+ /* Has the worker attached to the error message queue? */
+ mq = shm_mq_get_queue(worker->error_mqh);
+ if (shm_mq_get_sender(mq) != NULL)
+ worker_attached = true;
+ }
+ else if (status == BGWH_STOPPED)
+ {
+ /*
+ * If the worker stopped without attaching to the error message
+ * queue, throw an error. Otherwise it attached and reported an
+ * error before exiting, so mark it attached and let the next
+ * attempt to process pending messages, here or later while the
+ * initial snapshot is set up, throw that error.
+ */
+ mq = shm_mq_get_queue(worker->error_mqh);
+ if (shm_mq_get_sender(mq) == NULL)
+ 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."));
+
+ worker_attached = true;
+ }
+ else
+ {
+ /*
+ * Worker not yet started, so we must wait. The postmaster will
+ * notify us via bgw_notify_pid if its state changes.
+ */
+ rc = WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
+ -1, WAIT_EVENT_BGWORKER_STARTUP);
+
+ if (rc & WL_LATCH_SET)
+ ResetLatch(MyLatch);
+ }
+ }
+}
+
/*
* Stop the decoding worker and cleanup the related resources.
*
@@ -3714,13 +3822,47 @@ stop_repack_decoding_worker(void)
if (decoding_worker == NULL)
return;
- /* Terminate the worker process, if one is running. */
+ /* Terminate the worker and forget its error message queue. */
+ if (decoding_worker->error_mqh != NULL)
+ {
+ /*
+ * The error message queue is attached before the worker is
+ * registered.
+ */
+ if (decoding_worker->handle != NULL)
+ TerminateBackgroundWorker(decoding_worker->handle);
+
+ shm_mq_detach(decoding_worker->error_mqh);
+ decoding_worker->error_mqh = NULL;
+ }
+
+ /*
+ * Cancel any sleep on the condition variable before detaching the shared
+ * memory segment, because the CV lives in that segment. Otherwise later
+ * cleanup would touch freed memory.
+ */
+ ConditionVariableCancelSleep();
+
+ /*
+ * 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;
+ }
+
+ /*
+ * We can't finish the REPACK command until the worker has exited. This
+ * means, in particular, that we can't respond to interrupts at this
+ * stage.
+ */
if (decoding_worker->handle != NULL)
{
BgwHandleStatus status;
- TerminateBackgroundWorker(decoding_worker->handle);
- /* The worker should really exit before the REPACK command does. */
HOLD_INTERRUPTS();
status = WaitForBackgroundWorkerShutdown(decoding_worker->handle);
RESUME_INTERRUPTS();
@@ -3731,22 +3873,6 @@ stop_repack_decoding_worker(void)
errmsg("postmaster exited during REPACK command"));
}
- /*
- * Now detach from our shared memory segment. In error cases there might
- * still be messages from the worker in the queue, which ProcessInterrupts
- * would try to read; this is pointless (and causes an assertion failure),
- * so set the global pointer to NULL to have ProcessRepackMessages ignore
- * them.
- *
- * We must also cancel the current sleep, if one is still set up. This is
- * 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);
pfree(decoding_worker);
decoding_worker = NULL;
}
@@ -3851,9 +3977,11 @@ ProcessRepackMessages(void)
/*
* Nothing to do if we haven't launched the worker yet or have already
- * terminated it.
+ * terminated it. Stopping the worker detaches the error message 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;
/*
--
2.47.3
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-08 07:17 Antonin Houska <ah@cybertec.at>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Antonin Houska @ 2026-09-08 07:17 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-08 16:49 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 18+ messages in thread
From: Bharath Rupireddy @ 2026-09-08 16:49 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
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
Attachments:
[application/octet-stream] v4-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch (10.1K, ../../CALj2ACVCmocWuM8HTVdfE-AcgcO3wP5K6gFMh5CkDtbv4y2O9g@mail.gmail.com/2-v4-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch)
download | inline diff:
From 4f33dc2f10fa43d80a85ab138a85e8a3e09f53b6 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 8 Sep 2026 16:43:36 +0000
Subject: [PATCH v4] 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 message 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 CV, 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 for the worker to attach to the error message
queue before waiting for it to initialize decoding. The backend
waits on its process latch and checks the worker's status on each
wakeup. If the worker stopped without attaching, the backend
reports a startup failure. If it attached but reported an error
before exiting, that error is surfaced through the error message
queue. Only after the worker has started does the backend wait on
the CV for decoding to be initialized, so a worker that never
starts no longer causes a hang.
A second problem can occur when the backend stops the worker. It
waits for the worker to exit before detaching from the error
message queue. If the worker is blocked writing into a full
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 terminating the worker and detaching from the
error message queue before waiting for the worker to exit.
Detaching before the wait lets the blocked write fail so that the
worker can exit. On a failed worker registration the backend now
also forgets the error message queue, so teardown does not try to
terminate a worker that was never started.
Both fixes closely follow parallel.c, which coordinates its
workers through an error message queue in the same way.
These are less likely to hit in practice, but a user can trigger
these 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>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Reviewed-by: Masahiko Sawada <sawada.mshk@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 | 166 +++++++++++++++++++++++++++++-----
1 file changed, 145 insertions(+), 21 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 89aa038d23d..e5059f97924 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -216,6 +216,7 @@ static Oid determine_clustered_index(Relation rel, bool usingindex,
const char *indexname);
static void start_repack_decoding_worker(Oid relid);
+static void wait_for_repack_worker_to_attach(void);
static void stop_repack_decoding_worker(void);
static void stop_repack_decoding_worker_cb(int code, Datum arg);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
@@ -3684,10 +3685,34 @@ start_repack_decoding_worker(Oid relid)
bgw.bgw_notify_pid = MyProcPid;
if (!RegisterDynamicBackgroundWorker(&bgw, &decoding_worker->handle))
+ {
+ /*
+ * We couldn't register the worker, so forget the error message queue
+ * we set up for it. Otherwise stopping the worker later would try to
+ * terminate a worker that was never registered.
+ */
+ decoding_worker->handle = NULL;
+ shm_mq_detach(decoding_worker->error_mqh);
+ decoding_worker->error_mqh = NULL;
+
ereport(ERROR,
errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
errmsg("out of background worker slots"),
errhint("You might need to increase \"%s\".", "max_worker_processes"));
+ }
+
+ /*
+ * Now that the worker is registered, connect the error message queue to
+ * it.
+ */
+ shm_mq_set_handle(decoding_worker->error_mqh, decoding_worker->handle);
+
+ /*
+ * Make sure the worker has started before we wait for it to initialize
+ * decoding below, so that the failure-to-start case does not hang
+ * forever.
+ */
+ wait_for_repack_worker_to_attach();
/*
* The decoding setup must be done before the caller can have XID assigned
@@ -3712,6 +3737,89 @@ start_repack_decoding_worker(Oid relid)
ConditionVariableCancelSleep();
}
+/*
+ * Wait for the decoding worker to start up, and throw an error if it fails
+ * to do so.
+ *
+ * This is similar to WaitForParallelWorkersToAttach(). The only reliable way
+ * to tell a worker that failed to start (fork failure, or an exit before it
+ * attached) from one that is merely slow is to check whether it became the
+ * sender on the error message queue. If it stopped without attaching, nothing
+ * was queued and we report the generic failure ourselves. If it attached, any
+ * error it reported is in the queue and is thrown when we process pending
+ * messages, either here or later while we wait for it to initialize decoding.
+ */
+static void
+wait_for_repack_worker_to_attach(void)
+{
+ bool worker_attached = false;
+
+ for (;;)
+ {
+ BgwHandleStatus status;
+ shm_mq *mq;
+ int rc;
+ pid_t pid;
+
+ /*
+ * This will process any repack messages that are pending and it may
+ * also throw an error propagated from a worker.
+ */
+ CHECK_FOR_INTERRUPTS();
+
+ /* If the worker is known to have attached, we're done. */
+ if (worker_attached)
+ break;
+
+ /* If error_mqh is NULL, the worker has already exited cleanly. */
+ if (decoding_worker->error_mqh == NULL)
+ {
+ worker_attached = true;
+ continue;
+ }
+
+ status = GetBackgroundWorkerPid(decoding_worker->handle, &pid);
+ if (status == BGWH_STARTED)
+ {
+ /* Has the worker attached to the error message queue? */
+ mq = shm_mq_get_queue(decoding_worker->error_mqh);
+ if (shm_mq_get_sender(mq) != NULL)
+ worker_attached = true;
+ }
+ else if (status == BGWH_STOPPED)
+ {
+ /*
+ * If the worker stopped without attaching to the error message
+ * queue, throw an error. Otherwise it attached and reported an
+ * error before exiting, so mark it attached and let the next
+ * attempt to process pending messages, here or later while the
+ * initial snapshot is set up, throw that error.
+ */
+ mq = shm_mq_get_queue(decoding_worker->error_mqh);
+ if (shm_mq_get_sender(mq) == NULL)
+ 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."));
+
+ worker_attached = true;
+ }
+ else
+ {
+ /*
+ * Worker not yet started, so we must wait. The postmaster will
+ * notify us via bgw_notify_pid if its state changes.
+ */
+ rc = WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
+ -1, WAIT_EVENT_BGWORKER_STARTUP);
+
+ if (rc & WL_LATCH_SET)
+ ResetLatch(MyLatch);
+ }
+ }
+}
+
/*
* Stop the decoding worker and cleanup the related resources.
*
@@ -3725,13 +3833,43 @@ stop_repack_decoding_worker(void)
if (decoding_worker == NULL)
return;
- /* Terminate the worker process, if one is running. */
+ /* Terminate the worker and forget its error message queue. */
+ if (decoding_worker->error_mqh != NULL)
+ {
+ /*
+ * The error message queue is attached before the worker is
+ * registered.
+ */
+ if (decoding_worker->handle != NULL)
+ TerminateBackgroundWorker(decoding_worker->handle);
+
+ shm_mq_detach(decoding_worker->error_mqh);
+ decoding_worker->error_mqh = NULL;
+ }
+
+ /*
+ * Cancel any sleep on the condition variable before detaching the shared
+ * memory segment, because the CV lives in that segment. Otherwise later
+ * cleanup would touch freed memory.
+ */
+ ConditionVariableCancelSleep();
+
+ /* If we have allocated a shared memory segment, detach it. */
+ if (decoding_worker->seg != NULL)
+ {
+ dsm_detach(decoding_worker->seg);
+ decoding_worker->seg = NULL;
+ }
+
+ /*
+ * We can't finish the REPACK command until the worker has exited. This
+ * means, in particular, that we can't respond to interrupts at this
+ * stage.
+ */
if (decoding_worker->handle != NULL)
{
BgwHandleStatus status;
- TerminateBackgroundWorker(decoding_worker->handle);
- /* The worker should really exit before the REPACK command does. */
HOLD_INTERRUPTS();
status = WaitForBackgroundWorkerShutdown(decoding_worker->handle);
RESUME_INTERRUPTS();
@@ -3742,22 +3880,6 @@ stop_repack_decoding_worker(void)
errmsg("postmaster exited during REPACK command"));
}
- /*
- * Now detach from our shared memory segment. In error cases there might
- * still be messages from the worker in the queue, which ProcessInterrupts
- * would try to read; this is pointless (and causes an assertion failure),
- * so set the global pointer to NULL to have ProcessRepackMessages ignore
- * them.
- *
- * We must also cancel the current sleep, if one is still set up. This is
- * 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);
pfree(decoding_worker);
decoding_worker = NULL;
}
@@ -3862,9 +3984,11 @@ ProcessRepackMessages(void)
/*
* Nothing to do if we haven't launched the worker yet or have already
- * terminated it.
+ * terminated it. Stopping the worker detaches the error message 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;
/*
--
2.47.3
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-13 18:37 Nikolay Samokhvalov <nik@postgres.ai>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 2 replies; 18+ messages in thread
From: Nikolay Samokhvalov @ 2026-09-13 18:37 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
On Mon, Sep 7, 2026 at 12:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@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.
Álvaro pointed me here from my thread:
https://postgr.es/m/CAM527d9A7FBXJn7A3w2VAPq0Gojm7tnUDFMh1uwJitGfi2kNeQ@mail.gmail.com
My AI harness tested v4. Silent worker exits still hang three
post-attachment waits. Mihail raised snapshot/replay liveness in
December 2025.
Snapshot repro on a test build configured with --enable-injection-points,
with v4 plus only the attached patch's test support:
create extension injection_points;
create table t (i int primary key);
insert into t select generate_series(1, 100);
set statement_timeout = '2s';
select injection_points_attach(
'repack-worker-before-snapshot-export',
'injection_points', 'injection_exit', null);
repack (concurrently) t;
This times out after worker exit. The attached patch applies atop v4.
As far as I can see, the current uncommitted patches cover overall 5 cases:
- startup before queue attach: Bharath v4
- full error queue teardown: Bharath v4
- silent exit after attach, before initialization: attached patch
- silent exit before snapshot export: attached patch
- silent exit before replay export: attached patch
Nik
On Sun, Sep 13, 2026 at 11:09 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> 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
Attachments:
[application/x-patch] 0001-repack-worker-post-attachment-exit-minimal.patch (16.3K, ../../CAM527d8cF-6azsRWOPaOUCmsdgMG=ene7-st91_NM7v0q3uLLg@mail.gmail.com/2-0001-repack-worker-post-attachment-exit-minimal.patch)
download | inline diff:
From d9349e69acb0541c5838fd2f8786b40617c7682c Mon Sep 17 00:00:00 2001
From: Nik Samokhvalov <nik@postgres.ai>
Date: Sun, 13 Sep 2026 08:32:29 -0700
Subject: [PATCH] Detect REPACK worker exits after error queue attachment
Queue attachment does not protect the subsequent initialization,
snapshot, and replay-file waits from a worker that exits without
reporting an error.
Wait on the leader's latch and check worker status in those predicate
loops. Read status before the shared predicate so completed work wins
over worker exit. Preserve queued errors before reporting a generic
failure.
Test silent exits in all three phases and ordinary errors in both
wait loops, with same-backend retry and checks of data and filenodes.
This is incremental to Bharath Rupireddy's v4 worker startup/teardown
patch; its attachment wait and teardown changes are retained.
Reported-by: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Discussion: https://postgr.es/m/CADzfLwXp4c-MJx7yVDxAGNNxPbX4o9dqyivxavtHvmUsdXYqBQ@mail.gmail.com
Discussion: https://postgr.es/m/CALj2ACVAxA9HxvFe8HSspTJ-UO4Aoz=kuQdZBeLrod0gqUxH3g@mail.gmail.com
---
src/backend/commands/repack.c | 133 +++++++++++-------
src/backend/commands/repack_worker.c | 4 +
src/test/modules/injection_points/Makefile | 1 +
.../expected/repack_worker_exit.out | 100 +++++++++++++
.../injection_points/injection_points.c | 10 ++
src/test/modules/injection_points/meson.build | 1 +
.../specs/repack_worker_exit.spec | 66 +++++++++
7 files changed, 267 insertions(+), 48 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/repack_worker_exit.out
create mode 100644 src/test/modules/injection_points/specs/repack_worker_exit.spec
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 93fddd9..0f62f1e 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -220,6 +220,8 @@ static void start_repack_decoding_worker(Oid relid);
static void wait_for_repack_worker_to_attach(void);
static void stop_repack_decoding_worker(void);
static void stop_repack_decoding_worker_cb(int code, Datum arg);
+static void wait_for_repack_worker_file(DecodingWorkerShared *shared,
+ int expected_file);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
static void ProcessRepackMessage(StringInfo msg);
@@ -3114,29 +3116,8 @@ process_concurrent_changes(XLogRecPtr end_of_wal, ChangeContext *chgcxt, bool do
shared->done = done;
SpinLockRelease(&shared->mutex);
- /*
- * The worker needs to finish processing of the current WAL record. Even
- * if it's idle, it'll need to close the output file. Thus we're likely to
- * wait, so prepare for sleep.
- */
- ConditionVariablePrepareToSleep(&shared->cv);
- for (;;)
- {
- int last_exported;
-
- SpinLockAcquire(&shared->mutex);
- last_exported = shared->last_exported;
- SpinLockRelease(&shared->mutex);
-
- /*
- * Has the worker exported the file we are waiting for?
- */
- if (last_exported == chgcxt->cc_file_seq)
- break;
-
- ConditionVariableSleep(&shared->cv, WAIT_EVENT_REPACK_WORKER_EXPORT);
- }
- ConditionVariableCancelSleep();
+ /* Wait for the worker to finish and close the requested file. */
+ wait_for_repack_worker_file(shared, chgcxt->cc_file_seq);
/* Open the file. */
DecodingWorkerFileName(fname, shared->relid, chgcxt->cc_file_seq);
@@ -3826,10 +3807,23 @@ start_repack_decoding_worker(Oid relid)
* waiting for the caller's transaction to end. Therefore wait here until
* the worker indicates that it has the logical decoding initialized.
*/
- ConditionVariablePrepareToSleep(&shared->cv);
for (;;)
{
+ BgwHandleStatus status;
bool initialized;
+ pid_t pid;
+
+ ResetLatch(MyLatch);
+ CHECK_FOR_INTERRUPTS();
+
+ /* See wait_for_repack_worker_file() for the wait protocol. */
+ ConditionVariablePrepareToSleep(&shared->cv);
+
+ /*
+ * Read status first, so a worker that initializes and then exits
+ * still satisfies the predicate below.
+ */
+ status = GetBackgroundWorkerPid(decoding_worker->handle, &pid);
SpinLockAcquire(&shared->mutex);
initialized = shared->initialized;
@@ -3838,7 +3832,21 @@ start_repack_decoding_worker(Oid relid)
if (initialized)
break;
- ConditionVariableSleep(&shared->cv, WAIT_EVENT_REPACK_WORKER_EXPORT);
+ if (status == BGWH_STOPPED)
+ {
+ /* Report a queued worker error, if one is available. */
+ ConditionVariableCancelSleep();
+ ProcessRepackMessages();
+
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("REPACK worker failed to initialize"),
+ errhint("More details may be available in the server log.")));
+ }
+
+ (void) WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
+ -1, WAIT_EVENT_REPACK_WORKER_EXPORT);
}
ConditionVariableCancelSleep();
}
@@ -3998,43 +4006,72 @@ stop_repack_decoding_worker_cb(int code, Datum arg)
}
/*
- * Get the initial snapshot from the decoding worker.
+ * Wait for the decoding worker to export a requested file.
+ *
+ * Worker exit sets our latch without signaling the CV, so wait on the latch
+ * directly. Re-register on the CV before each predicate check, since a CV
+ * signal removes us from its wait list and WaitLatch() does not re-add us.
*/
-static Snapshot
-get_initial_snapshot(DecodingWorker *worker)
+static void
+wait_for_repack_worker_file(DecodingWorkerShared *shared, int expected_file)
{
- DecodingWorkerShared *shared;
- char fname[MAXPGPATH];
- BufFile *file;
- Size snap_size;
- char *snap_space;
- Snapshot snapshot;
-
- shared = (DecodingWorkerShared *) dsm_segment_address(worker->seg);
-
- /*
- * The worker needs to initialize the logical decoding, which usually
- * takes some time. Therefore it makes sense to prepare for the sleep
- * first.
- */
- ConditionVariablePrepareToSleep(&shared->cv);
for (;;)
{
+ BgwHandleStatus status;
int last_exported;
+ pid_t pid;
+ ResetLatch(MyLatch);
+ CHECK_FOR_INTERRUPTS();
+ ConditionVariablePrepareToSleep(&shared->cv);
+
+ /*
+ * Read status before the counter, so a worker that publishes its
+ * final file and then exits still satisfies the predicate below.
+ */
+ status = GetBackgroundWorkerPid(decoding_worker->handle, &pid);
SpinLockAcquire(&shared->mutex);
last_exported = shared->last_exported;
SpinLockRelease(&shared->mutex);
- /*
- * Has the worker exported the file we are waiting for?
- */
- if (last_exported == WORKER_FILE_SNAPSHOT)
+ if (last_exported == expected_file)
break;
- ConditionVariableSleep(&shared->cv, WAIT_EVENT_REPACK_WORKER_EXPORT);
+ if (status == BGWH_STOPPED)
+ {
+ ConditionVariableCancelSleep();
+ /* Preserve any error the worker queued before exiting. */
+ ProcessRepackMessages();
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("REPACK worker exited before exporting requested data"),
+ errhint("More details may be available in the server log.")));
+ }
+
+ (void) WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
+ -1, WAIT_EVENT_REPACK_WORKER_EXPORT);
}
ConditionVariableCancelSleep();
+}
+
+/*
+ * Get the initial snapshot from the decoding worker.
+ */
+static Snapshot
+get_initial_snapshot(DecodingWorker *worker)
+{
+ DecodingWorkerShared *shared;
+ char fname[MAXPGPATH];
+ BufFile *file;
+ Size snap_size;
+ char *snap_space;
+ Snapshot snapshot;
+
+ shared = (DecodingWorkerShared *) dsm_segment_address(worker->seg);
+
+ /* Wait for the worker to export the initial snapshot. */
+ wait_for_repack_worker_file(shared, WORKER_FILE_SNAPSHOT);
/* Read the snapshot from a file. */
DecodingWorkerFileName(fname, shared->relid, WORKER_FILE_SNAPSHOT);
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index b4ba9cf..3e34088 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -26,6 +26,7 @@
#include "storage/ipc.h"
#include "storage/proc.h"
#include "tcop/tcopprot.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#define PGREPACK_PLUGIN "pgrepack"
@@ -100,6 +101,7 @@ RepackWorkerMain(Datum main_arg)
pq_redirect_to_shm_mq(seg, mqh);
pq_set_parallel_leader(shared->backend_pid,
shared->backend_proc_number);
+ INJECTION_POINT("repack-worker-after-error-queue-attach", NULL);
/*
* Connect to the database, skipping the connection authorization checks
@@ -147,6 +149,7 @@ RepackWorkerMain(Datum main_arg)
/* Build the initial snapshot and export it. */
snapshot = SnapBuildInitialSnapshot(decoding_ctx->snapshot_builder);
+ INJECTION_POINT("repack-worker-before-snapshot-export", NULL);
export_initial_snapshot(snapshot, shared);
/*
@@ -487,6 +490,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx,
/*
* Close the file so we can make it available to the backend.
*/
+ INJECTION_POINT("repack-worker-before-replay-export", NULL);
BufFileClose(dstate->file);
dstate->file = NULL;
SpinLockAcquire(&shared->mutex);
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 408a35c..81512df 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -19,6 +19,7 @@ ISOLATION = basic \
reindex_concurrently_deferred \
repack \
repack_decode \
+ repack_worker_exit \
repack_temporal \
repack_temporal_multirange \
repack_toast \
diff --git a/src/test/modules/injection_points/expected/repack_worker_exit.out b/src/test/modules/injection_points/expected/repack_worker_exit.out
new file mode 100644
index 0000000..df39844
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_worker_exit.out
@@ -0,0 +1,100 @@
+Parsed test spec with 1 sessions
+
+starting permutation: repack init_error repack detach_init repack snapshot_error repack detach_snapshot repack detach_replay check_failure check_data repack check_success check_data
+step repack: REPACK (CONCURRENTLY) repack_worker_exit;
+ERROR: REPACK worker failed to initialize
+step init_error:
+ SELECT injection_points_detach('repack-worker-after-error-queue-attach');
+ SELECT injection_points_attach('repack-worker-after-error-queue-attach', 'error');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step repack: REPACK (CONCURRENTLY) repack_worker_exit;
+ERROR: error triggered for injection point repack-worker-after-error-queue-attach
+step detach_init:
+ SELECT injection_points_detach('repack-worker-after-error-queue-attach');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+step repack: REPACK (CONCURRENTLY) repack_worker_exit;
+ERROR: REPACK worker exited before exporting requested data
+step snapshot_error:
+ SELECT injection_points_detach('repack-worker-before-snapshot-export');
+ SELECT injection_points_attach('repack-worker-before-snapshot-export', 'error');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step repack: REPACK (CONCURRENTLY) repack_worker_exit;
+ERROR: error triggered for injection point repack-worker-before-snapshot-export
+step detach_snapshot:
+ SELECT injection_points_detach('repack-worker-before-snapshot-export');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+step repack: REPACK (CONCURRENTLY) repack_worker_exit;
+ERROR: REPACK worker exited before exporting requested data
+step detach_replay:
+ SELECT injection_points_detach('repack-worker-before-replay-export');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+step check_failure:
+ SELECT pg_relation_filenode('repack_worker_exit') = node AS failure_unchanged
+ FROM original_node;
+
+failure_unchanged
+-----------------
+t
+(1 row)
+
+step check_data: SELECT * FROM repack_worker_exit ORDER BY i;
+i|j
+-+-----
+1|one
+2|two
+3|three
+(3 rows)
+
+step repack: REPACK (CONCURRENTLY) repack_worker_exit;
+step check_success:
+ SELECT pg_relation_filenode('repack_worker_exit') <> node AS success_rewritten
+ FROM original_node;
+
+success_rewritten
+-----------------
+t
+(1 row)
+
+step check_data: SELECT * FROM repack_worker_exit ORDER BY i;
+i|j
+-+-----
+1|one
+2|two
+3|three
+(3 rows)
+
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 1683498..d3a4647 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -80,6 +80,9 @@ extern PGDLLEXPORT void injection_notice(const char *name,
extern PGDLLEXPORT void injection_wait(const char *name,
const void *private_data,
void *arg);
+extern PGDLLEXPORT void injection_exit(const char *name,
+ const void *private_data,
+ void *arg);
/* track if injection points attached in this process are linked to it */
static bool injection_point_local = false;
@@ -222,6 +225,13 @@ injection_notice(const char *name, const void *private_data, void *arg)
elog(NOTICE, "notice triggered for injection point %s", name);
}
+/* Exit without reporting an error. */
+void
+injection_exit(const char *name, const void *private_data, void *arg)
+{
+ proc_exit(1);
+}
+
/*
* Error cleanup callback for injection point waits.
*/
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index a7b40e0..1484630 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -48,6 +48,7 @@ tests += {
'reindex_concurrently_deferred',
'repack',
'repack_decode',
+ 'repack_worker_exit',
'repack_temporal',
'repack_temporal_multirange',
'repack_toast',
diff --git a/src/test/modules/injection_points/specs/repack_worker_exit.spec b/src/test/modules/injection_points/specs/repack_worker_exit.spec
new file mode 100644
index 0000000..ca44614
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_worker_exit.spec
@@ -0,0 +1,66 @@
+# Exercise each post-attachment wait, then retry in the same backend.
+setup
+{
+ CREATE EXTENSION injection_points;
+ SELECT injection_points_attach('repack-worker-after-error-queue-attach',
+ 'injection_points', 'injection_exit', NULL);
+ SELECT injection_points_attach('repack-worker-before-snapshot-export',
+ 'injection_points', 'injection_exit', NULL);
+ SELECT injection_points_attach('repack-worker-before-replay-export',
+ 'injection_points', 'injection_exit', NULL);
+ CREATE TABLE repack_worker_exit(i int PRIMARY KEY, j text);
+ INSERT INTO repack_worker_exit VALUES (1, 'one'), (2, 'two'), (3, 'three');
+ CREATE TABLE original_node AS
+ SELECT pg_relation_filenode('repack_worker_exit') AS node;
+}
+
+teardown
+{
+ DROP TABLE repack_worker_exit, original_node;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+step repack { REPACK (CONCURRENTLY) repack_worker_exit; }
+step init_error
+{
+ SELECT injection_points_detach('repack-worker-after-error-queue-attach');
+ SELECT injection_points_attach('repack-worker-after-error-queue-attach', 'error');
+}
+step detach_init
+{
+ SELECT injection_points_detach('repack-worker-after-error-queue-attach');
+}
+step snapshot_error
+{
+ SELECT injection_points_detach('repack-worker-before-snapshot-export');
+ SELECT injection_points_attach('repack-worker-before-snapshot-export', 'error');
+}
+step detach_snapshot
+{
+ SELECT injection_points_detach('repack-worker-before-snapshot-export');
+}
+step detach_replay
+{
+ SELECT injection_points_detach('repack-worker-before-replay-export');
+}
+step check_failure
+{
+ SELECT pg_relation_filenode('repack_worker_exit') = node AS failure_unchanged
+ FROM original_node;
+}
+step check_success
+{
+ SELECT pg_relation_filenode('repack_worker_exit') <> node AS success_rewritten
+ FROM original_node;
+}
+step check_data { SELECT * FROM repack_worker_exit ORDER BY i; }
+
+# A queued error must retain priority over the generic stopped-worker error.
+permutation
+ repack init_error
+ repack detach_init
+ repack snapshot_error
+ repack detach_snapshot
+ repack detach_replay check_failure check_data
+ repack check_success check_data
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-14 09:00 Antonin Houska <ah@cybertec.at>
parent: Nikolay Samokhvalov <nik@postgres.ai>
1 sibling, 1 reply; 18+ messages in thread
From: Antonin Houska @ 2026-09-14 09:00 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
Nikolay Samokhvalov <nik@postgres.ai> wrote:
> On Mon, Sep 7, 2026 at 12:15 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@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.
>
> Álvaro pointed me here from my thread:
> https://postgr.es/m/CAM527d9A7FBXJn7A3w2VAPq0Gojm7tnUDFMh1uwJitGfi2kNeQ@mail.gmail.com
>
> My AI harness tested v4. Silent worker exits still hang three
> post-attachment waits. Mihail raised snapshot/replay liveness in
> December 2025.
>
> Snapshot repro on a test build configured with --enable-injection-points,
> with v4 plus only the attached patch's test support:
>
> create extension injection_points;
> create table t (i int primary key);
> insert into t select generate_series(1, 100);
> set statement_timeout = '2s';
> select injection_points_attach(
> 'repack-worker-before-snapshot-export',
> 'injection_points', 'injection_exit', null);
> repack (concurrently) t;
>
> This times out after worker exit. The attached patch applies atop v4.
I'm not sure this should be considered a bug. AFAICS the problem happens due
to calling proc_exit() directly, i.e. exiting the worker w/o sending a message
via the error queue. (If an error was sent, ConditionVariableSleep() should
process it and raise ERROR in the backend running REPACK.) However, the REPACK
worker does not call proc_exit() directly.
(parallel.c tries to guard against worker calling proc_exit() and it seems to
have an issue, but it's a separate topic [1])
[1] https://www.postgresql.org/message-id/64338.1788881902%40localhost
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-15 01:31 Nikolay Samokhvalov <nik@postgres.ai>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 18+ messages in thread
From: Nikolay Samokhvalov @ 2026-09-15 01:31 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
On Mon, Sep 14, 2026 at 2:00 AM Antonin Houska <ah@cybertec.at> wrote:
> I'm not sure this should be considered a bug. AFAICS the problem happens
> due to calling proc_exit() directly, i.e. exiting the worker w/o sending a
> message via the error queue. (If an error was sent,
> ConditionVariableSleep() should process it and raise ERROR in the backend
> running REPACK.)
>
> However, the REPACK worker does not call proc_exit() directly.
Yes, agreed: the direct proc_exit() test alone didn't establish a bug. I
checked whether error delivery can actually fail after attachment.
There is a path in mq_putmessage(): an interrupt while blocked on the full
error queue makes recursive FATAL output detach it via pq_mq_busy. The FATAL
reaches the server log, but not the REPACK backend.
I reproduced this on v4 plus only the attached repack_worker.c test hooks,
built with injection points and server-wide client_min_messages=debug1:
1. For queue_t with a primary key, attach the standard wait action:
create extension injection_points;
select injection_points_attach(
'repack-worker-before-snapshot-export', 'wait');
2. In another session run repack (concurrently) queue_t. When the worker waits
at InjectionPoint/repack-worker-before-snapshot-export, SIGSTOP the REPACK
backend and run:
select injection_points_wakeup(
'repack-worker-before-snapshot-export');
3. Repeat until the worker waits at IPC/MessageQueuePutMessage:
select pg_logical_emit_message(false, 'repack-queue-test',
repeat('x', 15 * 1024 * 1024));
4. Run select pg_terminate_backend(worker_pid), wait for the worker to exit,
then SIGCONT the REPACK backend.
With v4, REPACK remains active in RepackWorkerExport. With the patch, it
returns 55000, "REPACK worker exited before exporting requested data".
So the bug is real and the worker-status check is needed. The direct
proc_exit() test was only an incomplete reproducer. Thanks for making me
check it.
Nik
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-15 08:38 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Nikolay Samokhvalov <nik@postgres.ai>
1 sibling, 0 replies; 18+ messages in thread
From: Bharath Rupireddy @ 2026-09-15 08:38 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: Antonin Houska <ah@cybertec.at>; Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
Hi,
On Sun, Sep 13, 2026 at 11:38 AM Nikolay Samokhvalov <nik@postgres.ai> wrote:
>
> My AI harness tested v4. Silent worker exits still hang three
> post-attachment waits. Mihail raised snapshot/replay liveness in
> December 2025.
>
> As far as I can see, the current uncommitted patches cover overall 5 cases:
>
> - startup before queue attach: Bharath v4
> - full error queue teardown: Bharath v4
> - silent exit after attach, before initialization: attached patch
> - silent exit before snapshot export: attached patch
> - silent exit before replay export: attached patch
Thanks a lot for sharing these additional corner cases. The first two
and the later three are separate problems, but I agree we need to fix
them all. I spent some time reading how parallel query handles these
and ended up with the attached patches.
0001 fixes the first two cases: the backend now waits for the worker
to attach to the error message queue before waiting for decoding to be
initialized, and detaches from that queue before waiting for the
worker to exit.
0002 fixes the three silent exit cases reported above: the worker now
sends a Terminate message as its last act, so the queue going away
without it means the worker left with the work unfinished.
0003 adds tests using injection points. I reused the one shared
upthread and just added cases for the hang and the deadlock that 0001
fixes. I don't intend to get this in unless others think it's
necessary.
0004 is a cleanup: the backend left the flag saying there are worker
messages to process set even when there was no worker to read messages
from, so every later interrupt check went looking for messages for
nothing.
Thoughts?
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
Attachments:
[application/octet-stream] v5-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch (9.5K, ../../CALj2ACWJsOG0-2SQUxBXG9ULFs_GJpeVhhmp8AHf8c9iKDjMXA@mail.gmail.com/2-v5-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch)
download | inline diff:
From 1bddaf2de146282ab023fc943f4a15563c1c2cdc Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 8 Sep 2026 16:43:36 +0000
Subject: [PATCH v5 1/4] Fix hang and deadlock in concurrent REPACK worker
handling.
REPACK CONCURRENTLY starts a background worker to decode the
changes made to the table while it is being repacked, and the
backend running the command coordinates with that worker through
shared memory and an error message queue.
Previously, the backend went straight to waiting on a condition
variable for the worker to set up decoding. Nothing in that wait
notices a worker that never arrived. The postmaster does signal
the backend when the worker fails to start, for example when
fork() fails, but the signal only wakes the sleep, which resumes
without raising anything. The backend therefore waits forever
while holding its lock on the table.
Fix this by waiting for the worker to attach to the error message
queue before waiting for it to initialize decoding. The worker's
status tells a worker that is gone from one that is merely slow,
and whether it ever became the sender on the queue tells whether
it left an error behind. A worker that stopped without attaching
gets the generic startup failure, and one that attached and
reported an error before exiting has that error come through the
queue.
The backend can also deadlock with the worker while stopping it,
because it waits for the worker to exit before detaching from the
error message queue. A worker blocked writing into a full queue
normally escapes once its send loop acts on the termination, but
not if it holds interrupts where it blocks, and the backend holds
interrupts of its own across the exit wait. Neither side can be
canceled.
Fix this by detaching from the error message queue before waiting
for the worker to exit, so that the blocked send fails with
SHM_MQ_DETACHED and the worker leaves without having to act on an
interrupt at all. Teardown only terminates a worker it has a
handle for, so a worker that was never registered needs nothing
undone.
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>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Reviewed-by: Masahiko Sawada <sawada.mshk@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 | 155 +++++++++++++++++++++++++++++-----
1 file changed, 134 insertions(+), 21 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 609e2c1d77e..068c6222e3d 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -217,6 +217,7 @@ static Oid determine_clustered_index(Relation rel, bool usingindex,
const char *indexname);
static void start_repack_decoding_worker(Oid relid);
+static void wait_for_repack_worker_to_attach(void);
static void stop_repack_decoding_worker(void);
static void stop_repack_decoding_worker_cb(int code, Datum arg);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
@@ -3795,6 +3796,19 @@ start_repack_decoding_worker(Oid relid)
errmsg("out of background worker slots"),
errhint("You might need to increase \"%s\".", "max_worker_processes"));
+ /*
+ * Now that the worker is registered, connect the error message queue to
+ * it.
+ */
+ shm_mq_set_handle(decoding_worker->error_mqh, decoding_worker->handle);
+
+ /*
+ * Make sure the worker has started before we wait for it to initialize
+ * decoding below, so that the failure-to-start case does not hang
+ * forever.
+ */
+ wait_for_repack_worker_to_attach();
+
/*
* 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,
@@ -3818,6 +3832,89 @@ start_repack_decoding_worker(Oid relid)
ConditionVariableCancelSleep();
}
+/*
+ * Wait for the decoding worker to start up, and throw an error if it fails
+ * to do so.
+ *
+ * This is similar to WaitForParallelWorkersToAttach(). The only reliable way
+ * to tell a worker that failed to start (fork failure, or an exit before it
+ * attached) from one that is merely slow is to check whether it became the
+ * sender on the error message queue. If it stopped without attaching, nothing
+ * was queued and we report the generic failure ourselves. If it attached, any
+ * error it reported is in the queue and is thrown when we process pending
+ * messages, either here or later while we wait for it to initialize decoding.
+ */
+static void
+wait_for_repack_worker_to_attach(void)
+{
+ bool worker_attached = false;
+
+ for (;;)
+ {
+ BgwHandleStatus status;
+ shm_mq *mq;
+ int rc;
+ pid_t pid;
+
+ /*
+ * This will process any repack messages that are pending and it may
+ * also throw an error propagated from a worker.
+ */
+ CHECK_FOR_INTERRUPTS();
+
+ /* If the worker is known to have attached, we're done. */
+ if (worker_attached)
+ break;
+
+ /* If error_mqh is NULL, the worker has already exited cleanly. */
+ if (decoding_worker->error_mqh == NULL)
+ {
+ worker_attached = true;
+ continue;
+ }
+
+ status = GetBackgroundWorkerPid(decoding_worker->handle, &pid);
+ if (status == BGWH_STARTED)
+ {
+ /* Has the worker attached to the error message queue? */
+ mq = shm_mq_get_queue(decoding_worker->error_mqh);
+ if (shm_mq_get_sender(mq) != NULL)
+ worker_attached = true;
+ }
+ else if (status == BGWH_STOPPED)
+ {
+ /*
+ * If the worker stopped without attaching to the error message
+ * queue, throw an error. Otherwise it attached and reported an
+ * error before exiting, so mark it attached and let the next
+ * attempt to process pending messages, here or later while the
+ * initial snapshot is set up, throw that error.
+ */
+ mq = shm_mq_get_queue(decoding_worker->error_mqh);
+ if (shm_mq_get_sender(mq) == NULL)
+ 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."));
+
+ worker_attached = true;
+ }
+ else
+ {
+ /*
+ * Worker not yet started, so we must wait. The postmaster will
+ * notify us via bgw_notify_pid if its state changes.
+ */
+ rc = WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
+ -1, WAIT_EVENT_BGWORKER_STARTUP);
+
+ if (rc & WL_LATCH_SET)
+ ResetLatch(MyLatch);
+ }
+ }
+}
+
/*
* Stop the decoding worker and cleanup the related resources.
*
@@ -3831,13 +3928,43 @@ stop_repack_decoding_worker(void)
if (decoding_worker == NULL)
return;
- /* Terminate the worker process, if one is running. */
+ /* Terminate the worker and forget its error message queue. */
+ if (decoding_worker->error_mqh != NULL)
+ {
+ /*
+ * The error message queue is attached before the worker is
+ * registered.
+ */
+ if (decoding_worker->handle != NULL)
+ TerminateBackgroundWorker(decoding_worker->handle);
+
+ shm_mq_detach(decoding_worker->error_mqh);
+ decoding_worker->error_mqh = NULL;
+ }
+
+ /*
+ * Cancel any sleep on the condition variable before detaching the shared
+ * memory segment, because the CV lives in that segment. Otherwise later
+ * cleanup would touch freed memory.
+ */
+ ConditionVariableCancelSleep();
+
+ /* If we have allocated a shared memory segment, detach it. */
+ if (decoding_worker->seg != NULL)
+ {
+ dsm_detach(decoding_worker->seg);
+ decoding_worker->seg = NULL;
+ }
+
+ /*
+ * We can't finish the REPACK command until the worker has exited. This
+ * means, in particular, that we can't respond to interrupts at this
+ * stage.
+ */
if (decoding_worker->handle != NULL)
{
BgwHandleStatus status;
- TerminateBackgroundWorker(decoding_worker->handle);
- /* The worker should really exit before the REPACK command does. */
HOLD_INTERRUPTS();
status = WaitForBackgroundWorkerShutdown(decoding_worker->handle);
RESUME_INTERRUPTS();
@@ -3848,22 +3975,6 @@ stop_repack_decoding_worker(void)
errmsg("postmaster exited during REPACK command"));
}
- /*
- * Now detach from our shared memory segment. In error cases there might
- * still be messages from the worker in the queue, which ProcessInterrupts
- * would try to read; this is pointless (and causes an assertion failure),
- * so set the global pointer to NULL to have ProcessRepackMessages ignore
- * them.
- *
- * We must also cancel the current sleep, if one is still set up. This is
- * 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);
pfree(decoding_worker);
decoding_worker = NULL;
}
@@ -3968,9 +4079,11 @@ ProcessRepackMessages(void)
/*
* Nothing to do if we haven't launched the worker yet or have already
- * terminated it.
+ * terminated it. Stopping the worker detaches the error message 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;
/*
--
2.47.3
[application/octet-stream] v5-0002-Detect-premature-exit-of-the-REPACK-decoding-work.patch (7.3K, ../../CALj2ACWJsOG0-2SQUxBXG9ULFs_GJpeVhhmp8AHf8c9iKDjMXA@mail.gmail.com/3-v5-0002-Detect-premature-exit-of-the-REPACK-decoding-work.patch)
download | inline diff:
From b6cfb97d3d473c2b43522a8f4ca98dc83cb33107 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 15 Sep 2026 06:25:58 +0000
Subject: [PATCH v5 2/4] Detect premature exit of the REPACK decoding worker.
Previously, the backend running REPACK CONCURRENTLY took the
decoding worker's error message queue going away as the normal
end of the worker's work, because the worker exits as soon as it
is done. It therefore could not tell that end from a worker that
went away early, and it has no other way of noticing one: it
waits for the worker in condition variable sleeps that wake only
on the worker's own signal.
As a result, a worker that exits before it initializes decoding,
before it exports the initial snapshot, or before it exports the
concurrent changes leaves the backend waiting forever for shared
state that nobody will set, while REPACK holds its lock on the
table. A worker that fails while the error message queue is full
ends up the same way, because its error report blocks in the
queue and is lost, so the failure only reaches the server log.
Fix this by having the worker say when it is done. The worker now
sends a Terminate message as its last act, and the backend
forgets the error message queue when it receives that message.
The queue going away without that message then means the worker
is gone with the work unfinished, and the backend reports it
instead of taking it for the normal end. The condition variable
loops need no change, because the next interrupt check after the
worker's signal throws the error.
For this to be reliable the worker has to detach from the shared
memory segment before it signals the backend. The other way
round, the backend can read the queue while the worker still
looks attached, and then nothing makes it read again.
Backpatch to 19, where REPACK CONCURRENTLY was introduced.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/CALj2ACVAxA9HxvFe8HSspTJ-UO4Aoz%3DkuQdZBeLrod0gqUxH3g%40mail.gmail.com
Discussion: https://postgr.es/m/CAM527d-bUOdoZezwXuhpjjwm-cB6q_m_YmJyVusgTPKSkohGJA%40mail.gmail.com
Backpatch-through: 19
---
src/backend/commands/repack.c | 37 +++++++++++++++++++++++-----
src/backend/commands/repack_worker.c | 29 +++++++++++++++-------
2 files changed, 51 insertions(+), 15 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 068c6222e3d..9c51b945a8a 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -4113,10 +4113,12 @@ ProcessRepackMessages(void)
RepackMessagePending = false;
/*
- * Read as many messages as we can from the worker, but stop when no more
- * messages can be read from the worker without blocking.
+ * Read as many messages as we can from the worker, but stop when either
+ * (1) the worker's error message queue goes away, which can happen if we
+ * receive a Terminate message from the worker; or (2) no more messages
+ * can be read from the worker without blocking.
*/
- while (true)
+ while (decoding_worker->error_mqh != NULL)
{
shm_mq_result res;
Size nbytes;
@@ -4138,12 +4140,22 @@ ProcessRepackMessages(void)
else
{
/*
- * The decoding worker is special in that it exits as soon as it
- * has its work done. Thus the DETACHED result code is fine.
+ * The worker detaches the error message queue when it exits, and
+ * a worker that finished its work told us so with a Terminate
+ * message. So the queue going away without that message means the
+ * worker is gone with the work unfinished, and we must report it
+ * here. Otherwise the REPACK command would wait forever for a
+ * worker that will never answer.
+ *
+ * The worker may well have failed with an error of its own that
+ * never reached us, so point to the server log for details.
*/
Assert(res == SHM_MQ_DETACHED);
- break;
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("lost connection to REPACK decoding worker"),
+ errhint("More details may be available in the server log."));
}
}
@@ -4195,6 +4207,19 @@ ProcessRepackMessage(StringInfo msg)
break;
}
+ case PqMsg_Terminate:
+ {
+ /*
+ * The worker sends this once it has finished its work and is
+ * about to exit, so stop watching its error message queue.
+ * The queue going away then no longer means that the worker
+ * is gone with the work unfinished.
+ */
+ shm_mq_detach(decoding_worker->error_mqh);
+ decoding_worker->error_mqh = NULL;
+ break;
+ }
+
default:
{
elog(ERROR, "unrecognized message type received from decoding worker: %c (message length %d bytes)",
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index bf2bc2dca13..bc30346861e 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -21,6 +21,7 @@
#include "access/xlogwait.h"
#include "commands/repack.h"
#include "commands/repack_internal.h"
+#include "libpq/libpq.h"
#include "libpq/pqmq.h"
#include "replication/snapbuild.h"
#include "storage/ipc.h"
@@ -44,8 +45,9 @@ static bool am_repack_worker = false;
/* The WAL segment being decoded. */
static XLogSegNo repack_current_segment = 0;
-/* Our DSM segment, for shutting down */
-static dsm_segment *worker_dsm_segment = NULL;
+/* Backend that launched us, for shutting down */
+static pid_t repack_backend_pid;
+static ProcNumber repack_backend_proc_number;
/*
* Keep track of the table we're processing, to skip logical decoding of data
@@ -76,12 +78,13 @@ RepackWorkerMain(Datum main_arg)
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("could not map dynamic shared memory segment"));
- worker_dsm_segment = seg;
shared = (DecodingWorkerShared *) dsm_segment_address(seg);
/* Arrange to signal the leader if we exit. */
- before_shmem_exit(RepackWorkerShutdown, PointerGetDatum(shared));
+ repack_backend_pid = shared->backend_pid;
+ repack_backend_proc_number = shared->backend_proc_number;
+ before_shmem_exit(RepackWorkerShutdown, PointerGetDatum(seg));
/*
* Join locking group - see the comments around the call of
@@ -167,6 +170,9 @@ RepackWorkerMain(Datum main_arg)
/* Cleanup. */
repack_cleanup_logical_decoding(decoding_ctx);
CommitTransactionCommand();
+
+ /* Report success, so that our exit does not look like a failure. */
+ pq_putmessage(PqMsg_Terminate, NULL, 0);
}
/*
@@ -175,13 +181,18 @@ RepackWorkerMain(Datum main_arg)
static void
RepackWorkerShutdown(int code, Datum arg)
{
- DecodingWorkerShared *shared = (DecodingWorkerShared *) DatumGetPointer(arg);
+ /*
+ * Detach from the shared memory segment before we signal the backend.
+ * Detaching also detaches the error message queue, and the backend learns
+ * that we are gone by reading that queue when it handles our signal. If
+ * we signaled first, the backend could read the queue while it still
+ * looks attached, and nothing would make it read again.
+ */
+ dsm_detach((dsm_segment *) DatumGetPointer(arg));
- SendProcSignal(shared->backend_pid,
+ SendProcSignal(repack_backend_pid,
PROCSIG_REPACK_MESSAGE,
- shared->backend_proc_number);
-
- dsm_detach(worker_dsm_segment);
+ repack_backend_proc_number);
}
bool
--
2.47.3
[application/octet-stream] v5-0003-Add-tests-for-a-REPACK-decoding-worker-that-goes-.patch (16.7K, ../../CALj2ACWJsOG0-2SQUxBXG9ULFs_GJpeVhhmp8AHf8c9iKDjMXA@mail.gmail.com/4-v5-0003-Add-tests-for-a-REPACK-decoding-worker-that-goes-.patch)
download | inline diff:
From 74ff3a7a3d827e5c5ee24b885e3711b47a402a69 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 15 Sep 2026 06:25:59 +0000
Subject: [PATCH v5 3/4] Add tests for a REPACK decoding worker that goes away.
Commits 1bddaf2de14 and e067c4ea39f taught the backend running
REPACK CONCURRENTLY to notice a decoding worker that went away
with the work unfinished, instead of waiting for it forever or
taking that for the normal end of the worker's work, but nothing
in the tree exercises that. The worker exits on its own as soon
as it is done, so the only way to reach those cases is to make it
leave while the backend is still waiting for it.
This adds injection points to the worker: one before it attaches
to the error message queue, one at each of the three places where
it has something to tell the backend, one right after the initial
snapshot is exported, and one in its exit callback, before it
detaches from the queue. It also adds two callbacks to the
injection_points module: injection_exit(), which calls
proc_exit() so that the worker leaves without reporting anything,
and injection_notice_oversized(), which emits a notice larger
than the error message queue so that the worker blocks writing
into it.
The test makes the worker exit before it attaches to the queue,
where nothing can tell the backend about it, and checks that the
backend reports the failure to start. It then makes the worker
exit silently at each of the three points and checks that the
backend reports the lost connection, makes it report an error at
each of those points and checks that the error reaches the
backend through the queue, and kills it while it blocks writing
into a full queue that nobody reads.
The deadlock that commit 1bddaf2de14 fixes needs a worker that
holds interrupts where it blocks writing into the queue, which no
SQL level failure arranges on its own. The test gets there
through the worker's exit path: proc_exit() holds interrupts and
clears the pending die and cancel flags, so anything the worker
sends from its exit callback goes out uninterruptibly. Cancelling
the backend while the worker waits at an injection point sends
the backend into teardown with the worker still alive, and the
oversized notice the worker emits on its way out then blocks in
the queue that only the backend could have drained.
Finally, the test checks that a failed REPACK leaves the table
alone, and that an undisturbed one rewrites it and keeps its data.
---
src/backend/commands/repack_worker.c | 12 +
.../injection_points/injection_points.c | 40 +++
src/test/modules/test_misc/meson.build | 1 +
.../modules/test_misc/t/100_repack_worker.pl | 240 ++++++++++++++++++
4 files changed, 293 insertions(+)
create mode 100644 src/test/modules/test_misc/t/100_repack_worker.pl
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index bc30346861e..a03c0f326d1 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -27,6 +27,7 @@
#include "storage/ipc.h"
#include "storage/proc.h"
#include "tcop/tcopprot.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#define PGREPACK_PLUGIN "pgrepack"
@@ -81,6 +82,9 @@ RepackWorkerMain(Datum main_arg)
shared = (DecodingWorkerShared *) dsm_segment_address(seg);
+ /* Leaving here means leaving silently: no error queue, no signal yet. */
+ INJECTION_POINT("repack-worker-before-error-queue-attach", NULL);
+
/* Arrange to signal the leader if we exit. */
repack_backend_pid = shared->backend_pid;
repack_backend_proc_number = shared->backend_proc_number;
@@ -104,6 +108,8 @@ RepackWorkerMain(Datum main_arg)
pq_set_parallel_leader(shared->backend_pid,
shared->backend_proc_number);
+ INJECTION_POINT("repack-worker-after-error-queue-attach", NULL);
+
/*
* Connect to the database, skipping the connection authorization checks
* as parallel workers do. Note that we run as the owner of the table
@@ -150,7 +156,9 @@ RepackWorkerMain(Datum main_arg)
/* Build the initial snapshot and export it. */
snapshot = SnapBuildInitialSnapshot(decoding_ctx->snapshot_builder);
+ INJECTION_POINT("repack-worker-before-snapshot-export", NULL);
export_initial_snapshot(snapshot, shared);
+ INJECTION_POINT("repack-worker-after-snapshot-export", NULL);
/*
* Only historic snapshots should be used now. Do not let us restrict the
@@ -181,6 +189,9 @@ RepackWorkerMain(Datum main_arg)
static void
RepackWorkerShutdown(int code, Datum arg)
{
+ /* Anything we send from here on goes out with interrupts held. */
+ INJECTION_POINT("repack-worker-before-exit", NULL);
+
/*
* Detach from the shared memory segment before we signal the backend.
* Detaching also detaches the error message queue, and the backend learns
@@ -498,6 +509,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx,
/*
* Close the file so we can make it available to the backend.
*/
+ INJECTION_POINT("repack-worker-before-changes-export", NULL);
BufFileClose(dstate->file);
dstate->file = NULL;
SpinLockAcquire(&shared->mutex);
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 66d8158d0c2..52afab59b51 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -41,6 +41,13 @@ PG_MODULE_MAGIC;
#define INJ_MAX_WAIT 8
#define INJ_NAME_MAXLEN 64
+/*
+ * Length of the notice emitted by injection_notice_oversized(), chosen to
+ * exceed the size of any shared message queue that a background worker uses
+ * to talk to the process that launched it.
+ */
+#define INJ_OVERSIZED_NOTICE_LEN (256 * 1024)
+
/* Thresholds of waits */
#define INJ_WAIT_INITIAL_US 10 /* 10us */
#define INJ_WAIT_MAX_US 100000 /* 100ms */
@@ -81,6 +88,12 @@ extern PGDLLEXPORT void injection_notice(const char *name,
extern PGDLLEXPORT void injection_wait(const char *name,
const void *private_data,
void *arg);
+extern PGDLLEXPORT void injection_exit(const char *name,
+ const void *private_data,
+ void *arg);
+extern PGDLLEXPORT void injection_notice_oversized(const char *name,
+ const void *private_data,
+ void *arg);
/* track if injection points attached in this process are linked to it */
static bool injection_point_local = false;
@@ -222,6 +235,33 @@ injection_notice(const char *name, const void *private_data, void *arg)
elog(NOTICE, "notice triggered for injection point %s", name);
}
+/*
+ * Exit the process without reporting anything, the way a process that calls
+ * proc_exit() directly does.
+ */
+void
+injection_exit(const char *name, const void *private_data, void *arg)
+{
+ proc_exit(1);
+}
+
+/*
+ * Emit a notice too large to fit into the queue a background worker sends its
+ * messages through, so that the worker blocks until the process that launched
+ * it reads from that queue.
+ */
+void
+injection_notice_oversized(const char *name, const void *private_data,
+ void *arg)
+{
+ char *message = palloc(INJ_OVERSIZED_NOTICE_LEN + 1);
+
+ memset(message, 'x', INJ_OVERSIZED_NOTICE_LEN);
+ message[INJ_OVERSIZED_NOTICE_LEN] = '\0';
+
+ elog(NOTICE, "%s", message);
+}
+
/*
* Error cleanup callback for injection point waits.
*/
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index 5d81f5b13be..1b3f248737d 100644
--- a/src/test/modules/test_misc/meson.build
+++ b/src/test/modules/test_misc/meson.build
@@ -24,6 +24,7 @@ tests += {
't/013_temp_obj_multisession.pl',
't/014_log_statement_max_length.pl',
't/015_temp_schema_exit_deferrable.pl',
+ 't/100_repack_worker.pl',
],
# The injection points are cluster-wide, so disable installcheck
'runningcheck': false,
diff --git a/src/test/modules/test_misc/t/100_repack_worker.pl b/src/test/modules/test_misc/t/100_repack_worker.pl
new file mode 100644
index 00000000000..077c109072f
--- /dev/null
+++ b/src/test/modules/test_misc/t/100_repack_worker.pl
@@ -0,0 +1,240 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# Test that REPACK (CONCURRENTLY) does not wait forever for a decoding worker
+# that goes away. The worker can leave before it can report anything at all,
+# leave after reporting an error, leave without a word, or be killed while it
+# blocks writing into a full error message queue, either while decoding or on
+# its way out.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+plan skip_all => 'Injection points not supported by this build'
+ unless $ENV{enable_injection_points} eq 'yes';
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+# REPACK (CONCURRENTLY) decodes WAL, so it needs more than wal_level=minimal.
+$node->init(allows_streaming => 1);
+$node->start;
+
+plan skip_all => 'Extension injection_points not installed'
+ unless $node->check_extension('injection_points');
+
+$node->safe_psql(
+ 'postgres', qq[
+ CREATE EXTENSION injection_points;
+ CREATE TABLE tbl (i int PRIMARY KEY, j text);
+ INSERT INTO tbl SELECT g, 'row ' || g FROM generate_series(1, 100) g;
+]);
+
+my $filenode =
+ $node->safe_psql('postgres', "SELECT pg_relation_filenode('tbl')");
+
+# Note that none of the injection points below is local to the session that
+# attaches it, because they all have to fire in the decoding worker.
+
+# A worker that leaves before it attaches to the error message queue leaves
+# without signalling the backend, so the queue cannot report it. The backend
+# has to notice it while waiting for it to start.
+{
+ my $point = 'repack-worker-before-error-queue-attach';
+
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_attach('$point', 'injection_points',
+ 'injection_exit', NULL);
+ ]);
+
+ my ($ret, $stdout, $stderr) =
+ $node->psql('postgres', 'REPACK (CONCURRENTLY) tbl');
+ isnt($ret, 0, "REPACK fails when the worker exits at $point");
+ like(
+ $stderr,
+ qr/REPACK decoding worker failed to start/,
+ "REPACK reports the worker that never attached to the queue");
+
+ $node->safe_psql('postgres', "SELECT injection_points_detach('$point')");
+}
+
+# These are the three places where the worker has something to tell the
+# backend, and thus the three places where the backend has something to wait
+# for: the decoding setup, the initial snapshot, and the file of concurrent
+# changes.
+my @points = (
+ 'repack-worker-after-error-queue-attach',
+ 'repack-worker-before-snapshot-export',
+ 'repack-worker-before-changes-export');
+
+# A worker that leaves without a word. The backend only learns about it from
+# the error message queue going away.
+foreach my $point (@points)
+{
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_attach('$point', 'injection_points',
+ 'injection_exit', NULL);
+ ]);
+
+ my ($ret, $stdout, $stderr) =
+ $node->psql('postgres', 'REPACK (CONCURRENTLY) tbl');
+ isnt($ret, 0, "REPACK fails when the worker exits at $point");
+ like(
+ $stderr,
+ qr/lost connection to REPACK decoding worker/,
+ "REPACK reports the worker that exited at $point");
+
+ $node->safe_psql('postgres', "SELECT injection_points_detach('$point')");
+}
+
+# A worker that reports an error before it leaves. The error reaches the
+# backend through the queue, so the backend reports that one instead.
+foreach my $point (@points)
+{
+ $node->safe_psql('postgres',
+ "SELECT injection_points_attach('$point', 'error')");
+
+ my ($ret, $stdout, $stderr) =
+ $node->psql('postgres', 'REPACK (CONCURRENTLY) tbl');
+ isnt($ret, 0, "REPACK fails when the worker errors out at $point");
+ like(
+ $stderr,
+ qr/error triggered for injection point $point/,
+ "REPACK reports the error of the worker at $point");
+
+ $node->safe_psql('postgres', "SELECT injection_points_detach('$point')");
+}
+
+is($node->safe_psql('postgres', "SELECT pg_relation_filenode('tbl')"),
+ $filenode, 'a failed REPACK leaves the table alone');
+
+# A worker killed while it waits for the backend to read from a full error
+# message queue. The error it reports on the way out cannot reach the backend,
+# because the queue it would go through is the one that is already full.
+SKIP:
+{
+ skip 'this test requires SIGSTOP', 1 if $windows_os;
+
+ # Hold the worker until we have stopped the backend, and then make it send
+ # a message far larger than the error message queue.
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_attach('repack-worker-before-snapshot-export',
+ 'wait');
+ SELECT injection_points_attach('repack-worker-after-snapshot-export',
+ 'injection_points',
+ 'injection_notice_oversized', NULL);
+ ]);
+
+ my $psql = $node->background_psql('postgres', on_error_stop => 0);
+ my $backend_pid = $psql->query('SELECT pg_backend_pid()');
+
+ $psql->{stdin} .= "REPACK (CONCURRENTLY) tbl;\n";
+ $psql->{run}->pump_nb();
+
+ # Once the worker is held, the backend is waiting for it and has not yet
+ # read anything from the queue.
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT count(*) = 1 FROM pg_stat_activity
+ WHERE backend_type = 'REPACK decoding worker'
+ AND wait_event = 'repack-worker-before-snapshot-export'
+ ]) or die "timed out while waiting for the decoding worker to start";
+
+ my $worker_pid = $node->safe_psql(
+ 'postgres', qq[
+ SELECT pid FROM pg_stat_activity
+ WHERE backend_type = 'REPACK decoding worker'
+ ]);
+
+ # Stop the backend, so that nothing reads from the queue any more, and let
+ # the worker fill it.
+ kill 'STOP', $backend_pid;
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_wakeup('repack-worker-before-snapshot-export');
+ ]);
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT count(*) = 1 FROM pg_stat_activity
+ WHERE pid = $worker_pid AND wait_event = 'MessageQueuePutMessage'
+ ]) or die "timed out while waiting for the error message queue to fill up";
+
+ # Kill the worker while it waits, then let the backend run again.
+ $node->safe_psql('postgres', "SELECT pg_terminate_backend($worker_pid)");
+ kill 'CONT', $backend_pid;
+
+ ok( pump_until(
+ $psql->{run}, $psql->{timeout},
+ \$psql->{stderr},
+ qr/lost connection to REPACK decoding worker/),
+ 'REPACK reports the worker killed while the queue was full');
+
+ $psql->quit;
+
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_detach('repack-worker-before-snapshot-export');
+ SELECT injection_points_detach('repack-worker-after-snapshot-export');
+ ]);
+}
+
+# A worker blocked writing into a full error message queue on its way out, where
+# proc_exit() holds interrupts and nothing can make it give up. The backend has
+# to stop watching the queue before it waits for the worker, or the two deadlock.
+{
+ # Hold the worker where the backend is waiting for it, and make it emit a
+ # message far larger than the error message queue as it exits.
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_attach('repack-worker-before-snapshot-export',
+ 'wait');
+ SELECT injection_points_attach('repack-worker-before-exit',
+ 'injection_points',
+ 'injection_notice_oversized', NULL);
+ ]);
+
+ my $psql = $node->background_psql('postgres', on_error_stop => 0);
+ my $backend_pid = $psql->query('SELECT pg_backend_pid()');
+
+ $psql->{stdin} .= "REPACK (CONCURRENTLY) tbl;\n";
+ $psql->{run}->pump_nb();
+
+ # While the worker is held, only it can export the snapshot the backend
+ # waits for, so the backend is asleep and reads nothing from the queue.
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT count(*) = 1 FROM pg_stat_activity
+ WHERE backend_type = 'REPACK decoding worker'
+ AND wait_event = 'repack-worker-before-snapshot-export'
+ ]) or die "timed out while waiting for the decoding worker to start";
+
+ # Cancel the backend, which sends it into teardown with the worker alive.
+ $node->safe_psql('postgres', "SELECT pg_cancel_backend($backend_pid)");
+
+ ok( pump_until(
+ $psql->{run}, $psql->{timeout},
+ \$psql->{stderr},
+ qr/canceling statement due to user request/),
+ 'REPACK stops the worker that blocks on the queue while exiting');
+
+ $psql->quit;
+
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_detach('repack-worker-before-snapshot-export');
+ SELECT injection_points_detach('repack-worker-before-exit');
+ ]);
+}
+
+# Nothing above left the table or the session in a state that keeps REPACK from
+# working.
+$node->safe_psql('postgres', 'REPACK (CONCURRENTLY) tbl');
+isnt($node->safe_psql('postgres', "SELECT pg_relation_filenode('tbl')"),
+ $filenode, 'REPACK rewrites the table once the worker is left alone');
+is($node->safe_psql('postgres', 'SELECT count(*), sum(i) FROM tbl'),
+ '100|5050', 'REPACK keeps the table data');
+
+done_testing();
--
2.47.3
[application/octet-stream] v5-0004-Clear-the-REPACK-message-flag-when-there-is-nothi.patch (2.1K, ../../CALj2ACWJsOG0-2SQUxBXG9ULFs_GJpeVhhmp8AHf8c9iKDjMXA@mail.gmail.com/5-v5-0004-Clear-the-REPACK-message-flag-when-there-is-nothi.patch)
download | inline diff:
From 87a4e6438f5d5a51f4844e83141319bac4b26786 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 15 Sep 2026 07:23:59 +0000
Subject: [PATCH v5 4/4] Clear the REPACK message flag when there is nothing to
do.
Previously, ProcessRepackMessages() returned without clearing
RepackMessagePending when there was no decoding worker to read
messages from. The worker signals the backend as it exits, and by
the time the backend handles that signal it has usually dropped
the worker already, so the flag stayed set. Every later
ProcessInterrupts() then called the function for nothing until
the session ran REPACK again.
Fix this by clearing the flag before that check, so that it
always tells whether there are messages to process. Nothing is
broken today, because the function has nothing to do without a
worker, but the flag should not claim work that is not there.
Backpatch to 19, where REPACK CONCURRENTLY was introduced.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/CALj2ACVAxA9HxvFe8HSspTJ-UO4Aoz%3DkuQdZBeLrod0gqUxH3g%40mail.gmail.com
Backpatch-through: 19
---
src/backend/commands/repack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 9c51b945a8a..d3bb296df25 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -4077,6 +4077,9 @@ ProcessRepackMessages(void)
MemoryContext oldcontext;
static MemoryContext hpm_context = NULL;
+ /* Reset the flag saying there are messages to process. */
+ RepackMessagePending = false;
+
/*
* Nothing to do if we haven't launched the worker yet or have already
* terminated it. Stopping the worker detaches the error message queue
@@ -4109,9 +4112,6 @@ ProcessRepackMessages(void)
oldcontext = MemoryContextSwitchTo(hpm_context);
- /* OK to process messages. Reset the flag saying there are more to do. */
- RepackMessagePending = false;
-
/*
* Read as many messages as we can from the worker, but stop when either
* (1) the worker's error message queue goes away, which can happen if we
--
2.47.3
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-15 10:26 Antonin Houska <ah@cybertec.at>
parent: Nikolay Samokhvalov <nik@postgres.ai>
0 siblings, 1 reply; 18+ messages in thread
From: Antonin Houska @ 2026-09-15 10:26 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
Nikolay Samokhvalov <nik@postgres.ai> wrote:
> On Mon, Sep 14, 2026 at 2:00 AM Antonin Houska <ah@cybertec.at> wrote:
> > I'm not sure this should be considered a bug. AFAICS the problem happens
> > due to calling proc_exit() directly, i.e. exiting the worker w/o sending a
> > message via the error queue. (If an error was sent,
> > ConditionVariableSleep() should process it and raise ERROR in the backend
> > running REPACK.)
> >
> > However, the REPACK worker does not call proc_exit() directly.
>
> Yes, agreed: the direct proc_exit() test alone didn't establish a bug. I
> checked whether error delivery can actually fail after attachment.
>
> There is a path in mq_putmessage(): an interrupt while blocked on the full
> error queue makes recursive FATAL output detach it via pq_mq_busy. The FATAL
> reaches the server log, but not the REPACK backend.
>
> I reproduced this on v4 plus only the attached repack_worker.c test hooks,
> built with injection points and server-wide client_min_messages=debug1:
>
> 1. For queue_t with a primary key, attach the standard wait action:
>
> create extension injection_points;
> select injection_points_attach(
> 'repack-worker-before-snapshot-export', 'wait');
>
> 2. In another session run repack (concurrently) queue_t. When the worker waits
> at InjectionPoint/repack-worker-before-snapshot-export, SIGSTOP the REPACK
> backend and run:
>
> select injection_points_wakeup(
> 'repack-worker-before-snapshot-export');
>
> 3. Repeat until the worker waits at IPC/MessageQueuePutMessage:
>
> select pg_logical_emit_message(false, 'repack-queue-test',
> repeat('x', 15 * 1024 * 1024));
>
> 4. Run select pg_terminate_backend(worker_pid), wait for the worker to exit,
> then SIGCONT the REPACK backend.
>
I wonder if you forgot to attach some files (I don't see the queue_t table in
the scripts you posted earlier), but I think I was able to reproduce the
problem using the instructions above.
IMO the problem is that the backend ignores the SHM_MQ_DETACHED state in
ProcessRepackMessages(). If all the messages in the queue have severity lower
than ERROR, the worker just processes them and continues waiting for the
worker to export the snapshot. However, as the worker is gone, there's no way
to wake the backend up.
Patch 0002 in [1] should fix that.
[1] https://www.postgresql.org/message-id/CALj2ACWJsOG0-2SQUxBXG9ULFs_GJpeVhhmp8AHf8c9iKDjMXA%40mail.gma...
Thanks for testing!
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-17 01:07 shihao zhong <zhong950419@gmail.com>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 18+ messages in thread
From: shihao zhong @ 2026-09-17 01:07 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Nikolay Samokhvalov <nik@postgres.ai>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers; alvherre@kurilemu.de
Hi Bharath,
I reviewed v5 patch by patch. v6 is attached
0001
It works. I made fork() really fail with a low RLIMIT_NPROC. On master,
REPACK (CONCURRENTLY) waits until statement_timeout. With 0001 it fails
at once with "REPACK decoding worker failed to start".
One problem. stop_repack_decoding_worker() now calls dsm_detach() before
it waits for the worker to exit. If the worker has mapped the segment
but not yet called SharedFileSetAttach(), the detach destroys the file
set, and the worker fails with
ERROR: could not attach to a SharedFileSet that is already destroyed
I hit this 12 times in 1200 cancelled REPACKs. Only shm_mq_detach() has
to come before the wait to fix the deadlock, so v6-0001 moves
dsm_detach() back after the wait, as on master. That gives 0 in 1200.
0002
Looks good. It follows parallel.c, and detaching before signalling is
needed, since with the old order the backend can read the queue too
early and never look again. With 0002 in, polling the worker status in
the wait loops, as Nikolay's patch does, is not needed.
No false alarms in 100 REPACK (CONCURRENTLY) runs with concurrent updates
and the owner's client_min_messages at debug1 and debug5, so the worker
often blocks on a full queue. pg_terminate_backend() on the worker gives
the backend the worker's own error at once.
0003
Please keep it. I checked that it catches each fix. Without the attach
wait, without the lost connection error, or with the queue detached
after the wait again, the test no longer passes.
One problem. The REPACK calls have no timeout, so a regression hangs
the test instead of failing it. Under meson it prints nothing until the
test is killed, and under make check nothing stops it. v6-0003 passes
timeout_default to those calls, so it now fails with "psql timed out"
at the right line. The rest of the test already waits with
timeout_default in poll_query_until(), pump_until() and
background_psql(), so this adds no new risk on slow machines.
0004
Looks fine. It matches what parallel.c does.
Thanks,
Shihao
Attachments:
[application/x-patch] v6-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch (10.0K, ../../CAGRkXqQwu8gFrpPXAkFSTOOK0bmf1MBSuLR6wENQH-JwcY4hRQ@mail.gmail.com/3-v6-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch)
download | inline diff:
From fe951ed8e53892a9ae545e1d65bb04477e57af70 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 8 Sep 2026 16:43:36 +0000
Subject: [PATCH v6 1/4] Fix hang and deadlock in concurrent REPACK worker
handling.
REPACK CONCURRENTLY starts a background worker to decode the
changes made to the table while it is being repacked, and the
backend running the command coordinates with that worker through
shared memory and an error message queue.
Previously, the backend went straight to waiting on a condition
variable for the worker to set up decoding. Nothing in that wait
notices a worker that never arrived. The postmaster does signal
the backend when the worker fails to start, for example when
fork() fails, but the signal only wakes the sleep, which resumes
without raising anything. The backend therefore waits forever
while holding its lock on the table.
Fix this by waiting for the worker to attach to the error message
queue before waiting for it to initialize decoding. The worker's
status tells a worker that is gone from one that is merely slow,
and whether it ever became the sender on the queue tells whether
it left an error behind. A worker that stopped without attaching
gets the generic startup failure, and one that attached and
reported an error before exiting has that error come through the
queue.
The backend can also deadlock with the worker while stopping it,
because it waits for the worker to exit before detaching from the
error message queue. A worker blocked writing into a full queue
normally escapes once its send loop acts on the termination, but
not if it holds interrupts where it blocks, and the backend holds
interrupts of its own across the exit wait. Neither side can be
canceled.
Fix this by detaching from the error message queue before waiting
for the worker to exit, so that the blocked send fails with
SHM_MQ_DETACHED and the worker leaves without having to act on an
interrupt at all. Teardown only terminates a worker it has a
handle for, so a worker that was never registered needs nothing
undone.
Only the queue is detached early. The shared memory segment is still
detached after the worker has exited, because the worker attaches to
the shared file set in that segment only after it has connected to
the database. Detaching the segment before that would destroy the
file set, and a worker that is still starting up would then fail
with "could not attach to a SharedFileSet that is already destroyed".
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>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@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 | 152 ++++++++++++++++++++++++++++++----
1 file changed, 135 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 3e972f2cc14..4d19dc06b66 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -217,6 +217,7 @@ static Oid determine_clustered_index(Relation rel, bool usingindex,
const char *indexname);
static void start_repack_decoding_worker(Oid relid);
+static void wait_for_repack_worker_to_attach(void);
static void stop_repack_decoding_worker(void);
static void stop_repack_decoding_worker_cb(int code, Datum arg);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
@@ -3796,6 +3797,19 @@ start_repack_decoding_worker(Oid relid)
errmsg("out of background worker slots"),
errhint("You might need to increase \"%s\".", "max_worker_processes"));
+ /*
+ * Now that the worker is registered, connect the error message queue to
+ * it.
+ */
+ shm_mq_set_handle(decoding_worker->error_mqh, decoding_worker->handle);
+
+ /*
+ * Make sure the worker has started before we wait for it to initialize
+ * decoding below, so that the failure-to-start case does not hang
+ * forever.
+ */
+ wait_for_repack_worker_to_attach();
+
/*
* 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,
@@ -3819,6 +3833,89 @@ start_repack_decoding_worker(Oid relid)
ConditionVariableCancelSleep();
}
+/*
+ * Wait for the decoding worker to start up, and throw an error if it fails
+ * to do so.
+ *
+ * This is similar to WaitForParallelWorkersToAttach(). The only reliable way
+ * to tell a worker that failed to start (fork failure, or an exit before it
+ * attached) from one that is merely slow is to check whether it became the
+ * sender on the error message queue. If it stopped without attaching, nothing
+ * was queued and we report the generic failure ourselves. If it attached, any
+ * error it reported is in the queue and is thrown when we process pending
+ * messages, either here or later while we wait for it to initialize decoding.
+ */
+static void
+wait_for_repack_worker_to_attach(void)
+{
+ bool worker_attached = false;
+
+ for (;;)
+ {
+ BgwHandleStatus status;
+ shm_mq *mq;
+ int rc;
+ pid_t pid;
+
+ /*
+ * This will process any repack messages that are pending and it may
+ * also throw an error propagated from a worker.
+ */
+ CHECK_FOR_INTERRUPTS();
+
+ /* If the worker is known to have attached, we're done. */
+ if (worker_attached)
+ break;
+
+ /* If error_mqh is NULL, the worker has already exited cleanly. */
+ if (decoding_worker->error_mqh == NULL)
+ {
+ worker_attached = true;
+ continue;
+ }
+
+ status = GetBackgroundWorkerPid(decoding_worker->handle, &pid);
+ if (status == BGWH_STARTED)
+ {
+ /* Has the worker attached to the error message queue? */
+ mq = shm_mq_get_queue(decoding_worker->error_mqh);
+ if (shm_mq_get_sender(mq) != NULL)
+ worker_attached = true;
+ }
+ else if (status == BGWH_STOPPED)
+ {
+ /*
+ * If the worker stopped without attaching to the error message
+ * queue, throw an error. Otherwise it attached and reported an
+ * error before exiting, so mark it attached and let the next
+ * attempt to process pending messages, here or later while the
+ * initial snapshot is set up, throw that error.
+ */
+ mq = shm_mq_get_queue(decoding_worker->error_mqh);
+ if (shm_mq_get_sender(mq) == NULL)
+ 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."));
+
+ worker_attached = true;
+ }
+ else
+ {
+ /*
+ * Worker not yet started, so we must wait. The postmaster will
+ * notify us via bgw_notify_pid if its state changes.
+ */
+ rc = WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
+ -1, WAIT_EVENT_BGWORKER_STARTUP);
+
+ if (rc & WL_LATCH_SET)
+ ResetLatch(MyLatch);
+ }
+ }
+}
+
/*
* Stop the decoding worker and cleanup the related resources.
*
@@ -3832,13 +3929,36 @@ stop_repack_decoding_worker(void)
if (decoding_worker == NULL)
return;
- /* Terminate the worker process, if one is running. */
+ /* Terminate the worker and forget its error message queue. */
+ if (decoding_worker->error_mqh != NULL)
+ {
+ /*
+ * The error message queue is attached before the worker is
+ * registered.
+ */
+ if (decoding_worker->handle != NULL)
+ TerminateBackgroundWorker(decoding_worker->handle);
+
+ shm_mq_detach(decoding_worker->error_mqh);
+ decoding_worker->error_mqh = NULL;
+ }
+
+ /*
+ * Cancel any sleep on the condition variable before detaching the shared
+ * memory segment, because the CV lives in that segment. Otherwise later
+ * cleanup would touch freed memory.
+ */
+ ConditionVariableCancelSleep();
+
+ /*
+ * We can't finish the REPACK command until the worker has exited. This
+ * means, in particular, that we can't respond to interrupts at this
+ * stage.
+ */
if (decoding_worker->handle != NULL)
{
BgwHandleStatus status;
- TerminateBackgroundWorker(decoding_worker->handle);
- /* The worker should really exit before the REPACK command does. */
HOLD_INTERRUPTS();
status = WaitForBackgroundWorkerShutdown(decoding_worker->handle);
RESUME_INTERRUPTS();
@@ -3850,21 +3970,17 @@ stop_repack_decoding_worker(void)
}
/*
- * Now detach from our shared memory segment. In error cases there might
- * still be messages from the worker in the queue, which ProcessInterrupts
- * would try to read; this is pointless (and causes an assertion failure),
- * so set the global pointer to NULL to have ProcessRepackMessages ignore
- * them.
- *
- * We must also cancel the current sleep, if one is still set up. This is
- * 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.
+ * Detach from the shared memory segment only now that the worker is gone.
+ * The worker attaches to the shared file set well after it maps the
+ * segment, so detaching any earlier can destroy the file set under a
+ * worker that is still starting up.
*/
- if (decoding_worker->error_mqh != NULL)
- shm_mq_detach(decoding_worker->error_mqh);
- ConditionVariableCancelSleep();
if (decoding_worker->seg != NULL)
+ {
dsm_detach(decoding_worker->seg);
+ decoding_worker->seg = NULL;
+ }
+
pfree(decoding_worker);
decoding_worker = NULL;
}
@@ -3969,9 +4085,11 @@ ProcessRepackMessages(void)
/*
* Nothing to do if we haven't launched the worker yet or have already
- * terminated it.
+ * terminated it. Stopping the worker detaches the error message 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;
/*
--
2.37.1 (Apple Git-137.1)
[application/x-patch] v6-0002-Detect-premature-exit-of-the-REPACK-decoding-work.patch (7.3K, ../../CAGRkXqQwu8gFrpPXAkFSTOOK0bmf1MBSuLR6wENQH-JwcY4hRQ@mail.gmail.com/4-v6-0002-Detect-premature-exit-of-the-REPACK-decoding-work.patch)
download | inline diff:
From 5a7491c52e72afa0a001d340b159797a36538a86 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 15 Sep 2026 06:25:58 +0000
Subject: [PATCH v6 2/4] Detect premature exit of the REPACK decoding worker.
Previously, the backend running REPACK CONCURRENTLY took the
decoding worker's error message queue going away as the normal
end of the worker's work, because the worker exits as soon as it
is done. It therefore could not tell that end from a worker that
went away early, and it has no other way of noticing one: it
waits for the worker in condition variable sleeps that wake only
on the worker's own signal.
As a result, a worker that exits before it initializes decoding,
before it exports the initial snapshot, or before it exports the
concurrent changes leaves the backend waiting forever for shared
state that nobody will set, while REPACK holds its lock on the
table. A worker that fails while the error message queue is full
ends up the same way, because its error report blocks in the
queue and is lost, so the failure only reaches the server log.
Fix this by having the worker say when it is done. The worker now
sends a Terminate message as its last act, and the backend
forgets the error message queue when it receives that message.
The queue going away without that message then means the worker
is gone with the work unfinished, and the backend reports it
instead of taking it for the normal end. The condition variable
loops need no change, because the next interrupt check after the
worker's signal throws the error.
For this to be reliable the worker has to detach from the shared
memory segment before it signals the backend. The other way
round, the backend can read the queue while the worker still
looks attached, and then nothing makes it read again.
Backpatch to 19, where REPACK CONCURRENTLY was introduced.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/CALj2ACVAxA9HxvFe8HSspTJ-UO4Aoz%3DkuQdZBeLrod0gqUxH3g%40mail.gmail.com
Discussion: https://postgr.es/m/CAM527d-bUOdoZezwXuhpjjwm-cB6q_m_YmJyVusgTPKSkohGJA%40mail.gmail.com
Backpatch-through: 19
---
src/backend/commands/repack.c | 37 +++++++++++++++++++++++-----
src/backend/commands/repack_worker.c | 29 +++++++++++++++-------
2 files changed, 51 insertions(+), 15 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 4d19dc06b66..489c74b5d64 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -4119,10 +4119,12 @@ ProcessRepackMessages(void)
RepackMessagePending = false;
/*
- * Read as many messages as we can from the worker, but stop when no more
- * messages can be read from the worker without blocking.
+ * Read as many messages as we can from the worker, but stop when either
+ * (1) the worker's error message queue goes away, which can happen if we
+ * receive a Terminate message from the worker; or (2) no more messages
+ * can be read from the worker without blocking.
*/
- while (true)
+ while (decoding_worker->error_mqh != NULL)
{
shm_mq_result res;
Size nbytes;
@@ -4144,12 +4146,22 @@ ProcessRepackMessages(void)
else
{
/*
- * The decoding worker is special in that it exits as soon as it
- * has its work done. Thus the DETACHED result code is fine.
+ * The worker detaches the error message queue when it exits, and
+ * a worker that finished its work told us so with a Terminate
+ * message. So the queue going away without that message means the
+ * worker is gone with the work unfinished, and we must report it
+ * here. Otherwise the REPACK command would wait forever for a
+ * worker that will never answer.
+ *
+ * The worker may well have failed with an error of its own that
+ * never reached us, so point to the server log for details.
*/
Assert(res == SHM_MQ_DETACHED);
- break;
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("lost connection to REPACK decoding worker"),
+ errhint("More details may be available in the server log."));
}
}
@@ -4201,6 +4213,19 @@ ProcessRepackMessage(StringInfo msg)
break;
}
+ case PqMsg_Terminate:
+ {
+ /*
+ * The worker sends this once it has finished its work and is
+ * about to exit, so stop watching its error message queue.
+ * The queue going away then no longer means that the worker
+ * is gone with the work unfinished.
+ */
+ shm_mq_detach(decoding_worker->error_mqh);
+ decoding_worker->error_mqh = NULL;
+ break;
+ }
+
default:
{
elog(ERROR, "unrecognized message type received from decoding worker: %c (message length %d bytes)",
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index bf2bc2dca13..bc30346861e 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -21,6 +21,7 @@
#include "access/xlogwait.h"
#include "commands/repack.h"
#include "commands/repack_internal.h"
+#include "libpq/libpq.h"
#include "libpq/pqmq.h"
#include "replication/snapbuild.h"
#include "storage/ipc.h"
@@ -44,8 +45,9 @@ static bool am_repack_worker = false;
/* The WAL segment being decoded. */
static XLogSegNo repack_current_segment = 0;
-/* Our DSM segment, for shutting down */
-static dsm_segment *worker_dsm_segment = NULL;
+/* Backend that launched us, for shutting down */
+static pid_t repack_backend_pid;
+static ProcNumber repack_backend_proc_number;
/*
* Keep track of the table we're processing, to skip logical decoding of data
@@ -76,12 +78,13 @@ RepackWorkerMain(Datum main_arg)
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("could not map dynamic shared memory segment"));
- worker_dsm_segment = seg;
shared = (DecodingWorkerShared *) dsm_segment_address(seg);
/* Arrange to signal the leader if we exit. */
- before_shmem_exit(RepackWorkerShutdown, PointerGetDatum(shared));
+ repack_backend_pid = shared->backend_pid;
+ repack_backend_proc_number = shared->backend_proc_number;
+ before_shmem_exit(RepackWorkerShutdown, PointerGetDatum(seg));
/*
* Join locking group - see the comments around the call of
@@ -167,6 +170,9 @@ RepackWorkerMain(Datum main_arg)
/* Cleanup. */
repack_cleanup_logical_decoding(decoding_ctx);
CommitTransactionCommand();
+
+ /* Report success, so that our exit does not look like a failure. */
+ pq_putmessage(PqMsg_Terminate, NULL, 0);
}
/*
@@ -175,13 +181,18 @@ RepackWorkerMain(Datum main_arg)
static void
RepackWorkerShutdown(int code, Datum arg)
{
- DecodingWorkerShared *shared = (DecodingWorkerShared *) DatumGetPointer(arg);
+ /*
+ * Detach from the shared memory segment before we signal the backend.
+ * Detaching also detaches the error message queue, and the backend learns
+ * that we are gone by reading that queue when it handles our signal. If
+ * we signaled first, the backend could read the queue while it still
+ * looks attached, and nothing would make it read again.
+ */
+ dsm_detach((dsm_segment *) DatumGetPointer(arg));
- SendProcSignal(shared->backend_pid,
+ SendProcSignal(repack_backend_pid,
PROCSIG_REPACK_MESSAGE,
- shared->backend_proc_number);
-
- dsm_detach(worker_dsm_segment);
+ repack_backend_proc_number);
}
bool
--
2.37.1 (Apple Git-137.1)
[application/x-patch] v6-0003-Add-tests-for-a-REPACK-decoding-worker-that-goes-.patch (17.0K, ../../CAGRkXqQwu8gFrpPXAkFSTOOK0bmf1MBSuLR6wENQH-JwcY4hRQ@mail.gmail.com/5-v6-0003-Add-tests-for-a-REPACK-decoding-worker-that-goes-.patch)
download | inline diff:
From 346ddaadcfce9462253502273596c0a8d120496d Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 15 Sep 2026 06:25:59 +0000
Subject: [PATCH v6 3/4] Add tests for a REPACK decoding worker that goes away.
Commits 1bddaf2de14 and e067c4ea39f taught the backend running
REPACK CONCURRENTLY to notice a decoding worker that went away
with the work unfinished, instead of waiting for it forever or
taking that for the normal end of the worker's work, but nothing
in the tree exercises that. The worker exits on its own as soon
as it is done, so the only way to reach those cases is to make it
leave while the backend is still waiting for it.
This adds injection points to the worker: one before it attaches
to the error message queue, one at each of the three places where
it has something to tell the backend, one right after the initial
snapshot is exported, and one in its exit callback, before it
detaches from the queue. It also adds two callbacks to the
injection_points module: injection_exit(), which calls
proc_exit() so that the worker leaves without reporting anything,
and injection_notice_oversized(), which emits a notice larger
than the error message queue so that the worker blocks writing
into it.
The test makes the worker exit before it attaches to the queue,
where nothing can tell the backend about it, and checks that the
backend reports the failure to start. It then makes the worker
exit silently at each of the three points and checks that the
backend reports the lost connection, makes it report an error at
each of those points and checks that the error reaches the
backend through the queue, and kills it while it blocks writing
into a full queue that nobody reads.
The deadlock that commit 1bddaf2de14 fixes needs a worker that
holds interrupts where it blocks writing into the queue, which no
SQL level failure arranges on its own. The test gets there
through the worker's exit path: proc_exit() holds interrupts and
clears the pending die and cancel flags, so anything the worker
sends from its exit callback goes out uninterruptibly. Cancelling
the backend while the worker waits at an injection point sends
the backend into teardown with the worker still alive, and the
oversized notice the worker emits on its way out then blocks in
the queue that only the backend could have drained.
Finally, the test checks that a failed REPACK leaves the table
alone, and that an undisturbed one rewrites it and keeps its data.
---
src/backend/commands/repack_worker.c | 12 +
.../injection_points/injection_points.c | 40 +++
src/test/modules/test_misc/meson.build | 1 +
.../modules/test_misc/t/100_repack_worker.pl | 250 ++++++++++++++++++
4 files changed, 303 insertions(+)
create mode 100644 src/test/modules/test_misc/t/100_repack_worker.pl
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index bc30346861e..a03c0f326d1 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -27,6 +27,7 @@
#include "storage/ipc.h"
#include "storage/proc.h"
#include "tcop/tcopprot.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#define PGREPACK_PLUGIN "pgrepack"
@@ -81,6 +82,9 @@ RepackWorkerMain(Datum main_arg)
shared = (DecodingWorkerShared *) dsm_segment_address(seg);
+ /* Leaving here means leaving silently: no error queue, no signal yet. */
+ INJECTION_POINT("repack-worker-before-error-queue-attach", NULL);
+
/* Arrange to signal the leader if we exit. */
repack_backend_pid = shared->backend_pid;
repack_backend_proc_number = shared->backend_proc_number;
@@ -104,6 +108,8 @@ RepackWorkerMain(Datum main_arg)
pq_set_parallel_leader(shared->backend_pid,
shared->backend_proc_number);
+ INJECTION_POINT("repack-worker-after-error-queue-attach", NULL);
+
/*
* Connect to the database, skipping the connection authorization checks
* as parallel workers do. Note that we run as the owner of the table
@@ -150,7 +156,9 @@ RepackWorkerMain(Datum main_arg)
/* Build the initial snapshot and export it. */
snapshot = SnapBuildInitialSnapshot(decoding_ctx->snapshot_builder);
+ INJECTION_POINT("repack-worker-before-snapshot-export", NULL);
export_initial_snapshot(snapshot, shared);
+ INJECTION_POINT("repack-worker-after-snapshot-export", NULL);
/*
* Only historic snapshots should be used now. Do not let us restrict the
@@ -181,6 +189,9 @@ RepackWorkerMain(Datum main_arg)
static void
RepackWorkerShutdown(int code, Datum arg)
{
+ /* Anything we send from here on goes out with interrupts held. */
+ INJECTION_POINT("repack-worker-before-exit", NULL);
+
/*
* Detach from the shared memory segment before we signal the backend.
* Detaching also detaches the error message queue, and the backend learns
@@ -498,6 +509,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx,
/*
* Close the file so we can make it available to the backend.
*/
+ INJECTION_POINT("repack-worker-before-changes-export", NULL);
BufFileClose(dstate->file);
dstate->file = NULL;
SpinLockAcquire(&shared->mutex);
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 66d8158d0c2..52afab59b51 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -41,6 +41,13 @@ PG_MODULE_MAGIC;
#define INJ_MAX_WAIT 8
#define INJ_NAME_MAXLEN 64
+/*
+ * Length of the notice emitted by injection_notice_oversized(), chosen to
+ * exceed the size of any shared message queue that a background worker uses
+ * to talk to the process that launched it.
+ */
+#define INJ_OVERSIZED_NOTICE_LEN (256 * 1024)
+
/* Thresholds of waits */
#define INJ_WAIT_INITIAL_US 10 /* 10us */
#define INJ_WAIT_MAX_US 100000 /* 100ms */
@@ -81,6 +88,12 @@ extern PGDLLEXPORT void injection_notice(const char *name,
extern PGDLLEXPORT void injection_wait(const char *name,
const void *private_data,
void *arg);
+extern PGDLLEXPORT void injection_exit(const char *name,
+ const void *private_data,
+ void *arg);
+extern PGDLLEXPORT void injection_notice_oversized(const char *name,
+ const void *private_data,
+ void *arg);
/* track if injection points attached in this process are linked to it */
static bool injection_point_local = false;
@@ -222,6 +235,33 @@ injection_notice(const char *name, const void *private_data, void *arg)
elog(NOTICE, "notice triggered for injection point %s", name);
}
+/*
+ * Exit the process without reporting anything, the way a process that calls
+ * proc_exit() directly does.
+ */
+void
+injection_exit(const char *name, const void *private_data, void *arg)
+{
+ proc_exit(1);
+}
+
+/*
+ * Emit a notice too large to fit into the queue a background worker sends its
+ * messages through, so that the worker blocks until the process that launched
+ * it reads from that queue.
+ */
+void
+injection_notice_oversized(const char *name, const void *private_data,
+ void *arg)
+{
+ char *message = palloc(INJ_OVERSIZED_NOTICE_LEN + 1);
+
+ memset(message, 'x', INJ_OVERSIZED_NOTICE_LEN);
+ message[INJ_OVERSIZED_NOTICE_LEN] = '\0';
+
+ elog(NOTICE, "%s", message);
+}
+
/*
* Error cleanup callback for injection point waits.
*/
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index 5d81f5b13be..1b3f248737d 100644
--- a/src/test/modules/test_misc/meson.build
+++ b/src/test/modules/test_misc/meson.build
@@ -24,6 +24,7 @@ tests += {
't/013_temp_obj_multisession.pl',
't/014_log_statement_max_length.pl',
't/015_temp_schema_exit_deferrable.pl',
+ 't/100_repack_worker.pl',
],
# The injection points are cluster-wide, so disable installcheck
'runningcheck': false,
diff --git a/src/test/modules/test_misc/t/100_repack_worker.pl b/src/test/modules/test_misc/t/100_repack_worker.pl
new file mode 100644
index 00000000000..29c0b1cf3f0
--- /dev/null
+++ b/src/test/modules/test_misc/t/100_repack_worker.pl
@@ -0,0 +1,250 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# Test that REPACK (CONCURRENTLY) does not wait forever for a decoding worker
+# that goes away. The worker can leave before it can report anything at all,
+# leave after reporting an error, leave without a word, or be killed while it
+# blocks writing into a full error message queue, either while decoding or on
+# its way out.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+plan skip_all => 'Injection points not supported by this build'
+ unless $ENV{enable_injection_points} eq 'yes';
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+# REPACK (CONCURRENTLY) decodes WAL, so it needs more than wal_level=minimal.
+$node->init(allows_streaming => 1);
+$node->start;
+
+plan skip_all => 'Extension injection_points not installed'
+ unless $node->check_extension('injection_points');
+
+$node->safe_psql(
+ 'postgres', qq[
+ CREATE EXTENSION injection_points;
+ CREATE TABLE tbl (i int PRIMARY KEY, j text);
+ INSERT INTO tbl SELECT g, 'row ' || g FROM generate_series(1, 100) g;
+]);
+
+my $filenode =
+ $node->safe_psql('postgres', "SELECT pg_relation_filenode('tbl')");
+
+# Note that none of the injection points below is local to the session that
+# attaches it, because they all have to fire in the decoding worker.
+#
+# Each REPACK below is given a timeout, so that a backend that waits forever
+# for the worker, which is the bug being tested, fails the test instead of
+# hanging it.
+
+# A worker that leaves before it attaches to the error message queue leaves
+# without signalling the backend, so the queue cannot report it. The backend
+# has to notice it while waiting for it to start.
+{
+ my $point = 'repack-worker-before-error-queue-attach';
+
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_attach('$point', 'injection_points',
+ 'injection_exit', NULL);
+ ]);
+
+ my ($ret, $stdout, $stderr) = $node->psql(
+ 'postgres',
+ 'REPACK (CONCURRENTLY) tbl',
+ timeout => $PostgreSQL::Test::Utils::timeout_default);
+ isnt($ret, 0, "REPACK fails when the worker exits at $point");
+ like(
+ $stderr,
+ qr/REPACK decoding worker failed to start/,
+ "REPACK reports the worker that never attached to the queue");
+
+ $node->safe_psql('postgres', "SELECT injection_points_detach('$point')");
+}
+
+# These are the three places where the worker has something to tell the
+# backend, and thus the three places where the backend has something to wait
+# for: the decoding setup, the initial snapshot, and the file of concurrent
+# changes.
+my @points = (
+ 'repack-worker-after-error-queue-attach',
+ 'repack-worker-before-snapshot-export',
+ 'repack-worker-before-changes-export');
+
+# A worker that leaves without a word. The backend only learns about it from
+# the error message queue going away.
+foreach my $point (@points)
+{
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_attach('$point', 'injection_points',
+ 'injection_exit', NULL);
+ ]);
+
+ my ($ret, $stdout, $stderr) = $node->psql(
+ 'postgres',
+ 'REPACK (CONCURRENTLY) tbl',
+ timeout => $PostgreSQL::Test::Utils::timeout_default);
+ isnt($ret, 0, "REPACK fails when the worker exits at $point");
+ like(
+ $stderr,
+ qr/lost connection to REPACK decoding worker/,
+ "REPACK reports the worker that exited at $point");
+
+ $node->safe_psql('postgres', "SELECT injection_points_detach('$point')");
+}
+
+# A worker that reports an error before it leaves. The error reaches the
+# backend through the queue, so the backend reports that one instead.
+foreach my $point (@points)
+{
+ $node->safe_psql('postgres',
+ "SELECT injection_points_attach('$point', 'error')");
+
+ my ($ret, $stdout, $stderr) = $node->psql(
+ 'postgres',
+ 'REPACK (CONCURRENTLY) tbl',
+ timeout => $PostgreSQL::Test::Utils::timeout_default);
+ isnt($ret, 0, "REPACK fails when the worker errors out at $point");
+ like(
+ $stderr,
+ qr/error triggered for injection point $point/,
+ "REPACK reports the error of the worker at $point");
+
+ $node->safe_psql('postgres', "SELECT injection_points_detach('$point')");
+}
+
+is($node->safe_psql('postgres', "SELECT pg_relation_filenode('tbl')"),
+ $filenode, 'a failed REPACK leaves the table alone');
+
+# A worker killed while it waits for the backend to read from a full error
+# message queue. The error it reports on the way out cannot reach the backend,
+# because the queue it would go through is the one that is already full.
+SKIP:
+{
+ skip 'this test requires SIGSTOP', 1 if $windows_os;
+
+ # Hold the worker until we have stopped the backend, and then make it send
+ # a message far larger than the error message queue.
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_attach('repack-worker-before-snapshot-export',
+ 'wait');
+ SELECT injection_points_attach('repack-worker-after-snapshot-export',
+ 'injection_points',
+ 'injection_notice_oversized', NULL);
+ ]);
+
+ my $psql = $node->background_psql('postgres', on_error_stop => 0);
+ my $backend_pid = $psql->query('SELECT pg_backend_pid()');
+
+ $psql->{stdin} .= "REPACK (CONCURRENTLY) tbl;\n";
+ $psql->{run}->pump_nb();
+
+ # Once the worker is held, the backend is waiting for it and has not yet
+ # read anything from the queue.
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT count(*) = 1 FROM pg_stat_activity
+ WHERE backend_type = 'REPACK decoding worker'
+ AND wait_event = 'repack-worker-before-snapshot-export'
+ ]) or die "timed out while waiting for the decoding worker to start";
+
+ my $worker_pid = $node->safe_psql(
+ 'postgres', qq[
+ SELECT pid FROM pg_stat_activity
+ WHERE backend_type = 'REPACK decoding worker'
+ ]);
+
+ # Stop the backend, so that nothing reads from the queue any more, and let
+ # the worker fill it.
+ kill 'STOP', $backend_pid;
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_wakeup('repack-worker-before-snapshot-export');
+ ]);
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT count(*) = 1 FROM pg_stat_activity
+ WHERE pid = $worker_pid AND wait_event = 'MessageQueuePutMessage'
+ ]) or die "timed out while waiting for the error message queue to fill up";
+
+ # Kill the worker while it waits, then let the backend run again.
+ $node->safe_psql('postgres', "SELECT pg_terminate_backend($worker_pid)");
+ kill 'CONT', $backend_pid;
+
+ ok( pump_until(
+ $psql->{run}, $psql->{timeout},
+ \$psql->{stderr},
+ qr/lost connection to REPACK decoding worker/),
+ 'REPACK reports the worker killed while the queue was full');
+
+ $psql->quit;
+
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_detach('repack-worker-before-snapshot-export');
+ SELECT injection_points_detach('repack-worker-after-snapshot-export');
+ ]);
+}
+
+# A worker blocked writing into a full error message queue on its way out, where
+# proc_exit() holds interrupts and nothing can make it give up. The backend has
+# to stop watching the queue before it waits for the worker, or the two deadlock.
+{
+ # Hold the worker where the backend is waiting for it, and make it emit a
+ # message far larger than the error message queue as it exits.
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_attach('repack-worker-before-snapshot-export',
+ 'wait');
+ SELECT injection_points_attach('repack-worker-before-exit',
+ 'injection_points',
+ 'injection_notice_oversized', NULL);
+ ]);
+
+ my $psql = $node->background_psql('postgres', on_error_stop => 0);
+ my $backend_pid = $psql->query('SELECT pg_backend_pid()');
+
+ $psql->{stdin} .= "REPACK (CONCURRENTLY) tbl;\n";
+ $psql->{run}->pump_nb();
+
+ # While the worker is held, only it can export the snapshot the backend
+ # waits for, so the backend is asleep and reads nothing from the queue.
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT count(*) = 1 FROM pg_stat_activity
+ WHERE backend_type = 'REPACK decoding worker'
+ AND wait_event = 'repack-worker-before-snapshot-export'
+ ]) or die "timed out while waiting for the decoding worker to start";
+
+ # Cancel the backend, which sends it into teardown with the worker alive.
+ $node->safe_psql('postgres', "SELECT pg_cancel_backend($backend_pid)");
+
+ ok( pump_until(
+ $psql->{run}, $psql->{timeout},
+ \$psql->{stderr},
+ qr/canceling statement due to user request/),
+ 'REPACK stops the worker that blocks on the queue while exiting');
+
+ $psql->quit;
+
+ $node->safe_psql(
+ 'postgres', qq[
+ SELECT injection_points_detach('repack-worker-before-snapshot-export');
+ SELECT injection_points_detach('repack-worker-before-exit');
+ ]);
+}
+
+# Nothing above left the table or the session in a state that keeps REPACK from
+# working.
+$node->safe_psql('postgres', 'REPACK (CONCURRENTLY) tbl');
+isnt($node->safe_psql('postgres', "SELECT pg_relation_filenode('tbl')"),
+ $filenode, 'REPACK rewrites the table once the worker is left alone');
+is($node->safe_psql('postgres', 'SELECT count(*), sum(i) FROM tbl'),
+ '100|5050', 'REPACK keeps the table data');
+
+done_testing();
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] v6-0004-Clear-the-REPACK-message-flag-when-there-is-nothi.patch (2.2K, ../../CAGRkXqQwu8gFrpPXAkFSTOOK0bmf1MBSuLR6wENQH-JwcY4hRQ@mail.gmail.com/6-v6-0004-Clear-the-REPACK-message-flag-when-there-is-nothi.patch)
download | inline diff:
From c3971b6ad1f334b1c6081ec14b14bbb25a89c717 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 15 Sep 2026 07:23:59 +0000
Subject: [PATCH v6 4/4] Clear the REPACK message flag when there is nothing to
do.
Previously, ProcessRepackMessages() returned without clearing
RepackMessagePending when there was no decoding worker to read
messages from. The worker signals the backend as it exits, and by
the time the backend handles that signal it has usually dropped
the worker already, so the flag stayed set. Every later
ProcessInterrupts() then called the function for nothing until
the session ran REPACK again.
Fix this by clearing the flag before that check, so that it
always tells whether there are messages to process. Nothing is
broken today, because the function has nothing to do without a
worker, but the flag should not claim work that is not there.
Backpatch to 19, where REPACK CONCURRENTLY was introduced.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/CALj2ACVAxA9HxvFe8HSspTJ-UO4Aoz%3DkuQdZBeLrod0gqUxH3g%40mail.gmail.com
Backpatch-through: 19
---
src/backend/commands/repack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 489c74b5d64..6b1eb2eaaba 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -4083,6 +4083,9 @@ ProcessRepackMessages(void)
MemoryContext oldcontext;
static MemoryContext hpm_context = NULL;
+ /* Reset the flag saying there are messages to process. */
+ RepackMessagePending = false;
+
/*
* Nothing to do if we haven't launched the worker yet or have already
* terminated it. Stopping the worker detaches the error message queue
@@ -4115,9 +4118,6 @@ ProcessRepackMessages(void)
oldcontext = MemoryContextSwitchTo(hpm_context);
- /* OK to process messages. Reset the flag saying there are more to do. */
- RepackMessagePending = false;
-
/*
* Read as many messages as we can from the worker, but stop when either
* (1) the worker's error message queue goes away, which can happen if we
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-17 09:53 Alvaro Herrera <alvherre@kurilemu.de>
parent: shihao zhong <zhong950419@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Alvaro Herrera @ 2026-09-17 09:53 UTC (permalink / raw)
To: shihao zhong <zhong950419@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; Nikolay Samokhvalov <nik@postgres.ai>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
Hello,
I have pushed these fixes, with some editorialization of my own.
I decided not to push the test case, because it has some nontrivial
additions to the injection point infrastructure that I'm not so sure
about. The part of the test that doesn't require those changes could
perhaps be considered for 19, and we can discuss adding the rest in 20.
Thanks!
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start
@ 2026-09-17 19:06 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 0 replies; 18+ messages in thread
From: Bharath Rupireddy @ 2026-09-17 19:06 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: shihao zhong <zhong950419@gmail.com>; Antonin Houska <ah@cybertec.at>; Nikolay Samokhvalov <nik@postgres.ai>; Masahiko Sawada <sawada.mshk@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
Hi,
On Thu, Sep 17, 2026 at 2:53 AM Alvaro Herrera <alvherre@kurilemu.de> wrote:
>
> Hello,
>
> I have pushed these fixes, with some editorialization of my own.
>
> I decided not to push the test case, because it has some nontrivial
> additions to the injection point infrastructure that I'm not so sure
> about. The part of the test that doesn't require those changes could
> perhaps be considered for 19, and we can discuss adding the rest in 20.
Thanks Alvaro for taking care of this. I think it is fine to not have
the tests in core and to have them here in the thread for reference.
My thinking is that the issues fixed here look rare in practice and
making them run reliably and deterministically would need more
developer cycles. Moreover, we haven't had such tests for parallel
query for a while, and testing resources are not free. Others may have
a different opinion.
And thanks Shihao for reviewing and identifying one of the problems
and proposing the fix for it.
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2026-09-17 19:06 UTC | newest]
Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-08-27 16:41 REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2026-08-27 16:47 ` Nathan Bossart <nathandbossart@gmail.com>
2026-08-28 20:20 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2026-08-31 10:31 ` Antonin Houska <ah@cybertec.at>
2026-08-31 18:14 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2026-09-05 00:11 ` Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-07 18:46 ` Antonin Houska <ah@cybertec.at>
2026-09-07 19:15 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2026-09-08 07:17 ` Antonin Houska <ah@cybertec.at>
2026-09-08 16:49 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2026-09-13 18:37 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-09-14 09:00 ` Antonin Houska <ah@cybertec.at>
2026-09-15 01:31 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-09-15 10:26 ` Antonin Houska <ah@cybertec.at>
2026-09-17 01:07 ` shihao zhong <zhong950419@gmail.com>
2026-09-17 09:53 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-09-17 19:06 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2026-09-15 08:38 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox