pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
8+ messages / 4 participants
[nested] [flat]
* [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
@ 2026-05-12 23:26 Baji Shaik <baji.pgdev@gmail.com>
2026-05-13 03:45 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Sami Imseih <samimseih@gmail.com>
2026-05-17 20:46 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 2 replies; 8+ messages in thread
From: Baji Shaik @ 2026-05-12 23:26 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: alvherre@kurilemu.de
Hi,
When a REPACK (CONCURRENTLY) session is terminated via
pg_terminate_backend(), the REPACK decoding worker keeps running
indefinitely and holds its temporary replication slot.
To reproduce:
-- Session 1: start a long REPACK
REPACK (CONCURRENTLY) big_table;
-- Session 2: kill it
SELECT pg_terminate_backend(pid)
FROM pg_stat_activity
WHERE query LIKE '%REPACK%';
-- The slot persists:
SELECT slot_name, active FROM pg_replication_slots;
-- repack_NNNN | t (still active, cannot be dropped)
Root cause:
pg_terminate_backend() causes ereport(FATAL) via ProcDiePending.
FATAL exits bypass PG_FINALLY blocks, so stop_repack_decoding_worker()
is never called. The decoding worker is left running.
Fix:
Register an on_proc_exit callback when the decoding worker starts.
The callback calls TerminateBackgroundWorker() to signal the worker.
We do not wait for the worker to exit in the callback (WaitLatch is
not safe during proc_exit); the worker's RS_TEMPORARY slot is dropped
automatically when the worker process exits.
Thanks,
Baji Shaik
AWS RDS
Attachments:
[application/octet-stream] 0001-Fix-REPACK-decoding-worker-not-cleaned-up-on-FATAL-e.patch (3.0K, ../../CA+fm-RNoPxL2N7db_A0anMXV_aDu6jWj4PNOPtMtBUAPDPvSXQ@mail.gmail.com/3-0001-Fix-REPACK-decoding-worker-not-cleaned-up-on-FATAL-e.patch)
download | inline diff:
From b5ffcb057da6914d7d5ac61e343d73b5102be8db Mon Sep 17 00:00:00 2001
From: Baji Shaik <baji.pgdev@gmail.com>
Date: Tue, 12 May 2026 01:27:46 +0000
Subject: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
When the launching backend of REPACK (CONCURRENTLY) is terminated
via pg_terminate_backend(), ProcDiePending causes ereport(FATAL)
which bypasses PG_FINALLY blocks. As a result, stop_repack_decoding_
worker() is never called, leaving the decoding worker running
indefinitely and holding its temporary replication slot.
Fix by registering an on_proc_exit callback when the decoding worker
is started. The callback signals the worker to terminate via
TerminateBackgroundWorker(). We do not wait for the worker to exit
in the callback, as WaitLatch is not safe during proc_exit; instead
we rely on the worker temporary replication slot (RS_TEMPORARY)
being dropped automatically when the worker process exits after
receiving the termination signal.
---
src/backend/commands/repack.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 860e2aecbe9..f4ba2d02348 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -64,6 +64,7 @@
#include "pgstat.h"
#include "replication/logicalrelation.h"
#include "storage/bufmgr.h"
+#include "storage/ipc.h"
#include "storage/lmgr.h"
#include "storage/predicate.h"
#include "storage/proc.h"
@@ -211,6 +212,7 @@ static Oid determine_clustered_index(Relation rel, bool usingindex,
static void start_repack_decoding_worker(Oid relid);
static void stop_repack_decoding_worker(void);
+static void repack_decoding_worker_exit_cleanup(int code, Datum arg);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
static void ProcessRepackMessage(StringInfo msg);
@@ -3454,6 +3456,8 @@ start_repack_decoding_worker(Oid relid)
decoding_worker->seg = seg;
decoding_worker->error_mqh = mqh;
+ on_proc_exit(repack_decoding_worker_exit_cleanup, (Datum) 0);
+
/*
* 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,
@@ -3477,6 +3481,25 @@ start_repack_decoding_worker(Oid relid)
ConditionVariableCancelSleep();
}
+/*
+ * proc_exit callback to stop the decoding worker on abnormal exit.
+ * PG_FINALLY does not run on FATAL, so this ensures the worker is
+ * terminated even when the launching backend is killed.
+ *
+ * We only send the termination signal here; we do not wait for the
+ * worker to exit, as waiting via WaitLatch is not safe during proc_exit.
+ * The temporary replication slot will be cleaned up when the worker
+ * exits on its own after receiving the signal.
+ */
+static void
+repack_decoding_worker_exit_cleanup(int code, Datum arg)
+{
+ if (decoding_worker == NULL)
+ return;
+ if (decoding_worker->handle != NULL)
+ TerminateBackgroundWorker(decoding_worker->handle);
+}
+
/*
* Stop the decoding worker and cleanup the related resources.
*
--
2.50.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
2026-05-12 23:26 [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
@ 2026-05-13 03:45 ` Sami Imseih <samimseih@gmail.com>
1 sibling, 0 replies; 8+ messages in thread
From: Sami Imseih @ 2026-05-13 03:45 UTC (permalink / raw)
To: Baji Shaik <baji.pgdev@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org; alvherre@kurilemu.de
Hi,
Thanks for reporting. This indeed looks like a bug.
With pg_terminate_backend, the logical replication worker has no
way to know that it needs to stop, as the PG_FINALLY is not
reached in this case.
I think registering a callback to terminate the worker is the proper fix,
but I don't think on_proc_exit() is the right place to register the
callback.
With 0001 applied and building with asserts, I see a segfault.
postgres=# select pg_terminate_backend(26707);
pg_terminate_backend
----------------------
t
(1 row)
```
postgres=# select 1;
WARNING: terminating connection because of crash of another server process
DETAIL: The postmaster has commanded this server process to roll back
the current transaction and exit, because another server process
exited abnormally and possibly corrupted shared memory.
HINT: In a moment you should be able to reconnect to the database and
repeat your command.
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
postgres=?#
```
```
2026-05-12 21:50:33.866 CDT [26569] LOG: client backend (PID 26707)
was terminated by signal 11: Segmentation fault: 11
2026-05-12 21:50:33.866 CDT [26569] LOG: terminating any other active
server processes
2026-05-12 21:50:33.872 CDT [26569] LOG: all server processes
terminated; reinitializing
2026-05-12 21:50:33.882 CDT [27131] LOG: database system was
interrupted; last known up at 2026-05-12 21:45:39 CDT
2026-05-12 21:50:34.278 CDT [27131] LOG: database system was not
properly shut down; automatic recovery in progress
2026-05-12 21:50:34.281 CDT [27131] LOG: redo starts at 13/619E9470
```
From lldb on my Mac, I see
```
Process 22683 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason =
EXC_BAD_ACCESS (code=1, address=0x7f7f7f7f7f7f7f7f)
frame #0: 0x00000001044c607c
postgres`TerminateBackgroundWorker(handle=0x7f7f7f7f7f7f7f7f) at
bgworker.c:1324:2 [opt]
1321 BackgroundWorkerSlot *slot;
1322 bool signal_postmaster = false;
1323
-> 1324 Assert(handle->slot < max_worker_processes);
1325 slot = &BackgroundWorkerData->slot[handle->slot];
1326
1327 /* Set terminate flag in shared memory, unless
slot has been reused. */
```
The 0x7f7f7f7f7f7f7f7f is the CLOBBER_FREED_MEMORY fill pattern from
wipe_mem(). The handle's memory context has already been destroyed by
the time on_proc_exit callbacks run.
A better fix is to use before_shmem_exit instead, which is for
user-level cleanup.
/* ----------------------------------------------------------------
* before_shmem_exit
*
* Register early callback to perform user-level cleanup,
If we do that, we can also wait for the worker to shutdown, so we can use
stop_repack_decoding_worker();
What do you think?
--
Sami Imseih
Amazon Web Services (AWS)
Attachments:
[application/octet-stream] v2-0001-Fix-REPACK-decoding-worker-not-cleaned-up-on-FATA.patch (2.6K, ../../CAA5RZ0sYXTGQK=JStyFv9p12sZk3Vc0_9E10HbgJa47CCoGfQQ@mail.gmail.com/2-v2-0001-Fix-REPACK-decoding-worker-not-cleaned-up-on-FATA.patch)
download | inline diff:
From edff42cb08bcd18c4e9b593ec23504532f7431f2 Mon Sep 17 00:00:00 2001
From: Baji Shaik <baji.pgdev@gmail.com>
Date: Tue, 12 May 2026 01:27:46 +0000
Subject: [PATCH v2 1/1] Fix REPACK decoding worker not cleaned up on FATAL
exit
When the launching backend of REPACK (CONCURRENTLY) is terminated
via pg_terminate_backend(), ProcDiePending causes ereport(FATAL)
which bypasses PG_FINALLY blocks. As a result,
stop_repack_decoding_worker() is never called, leaving the
decoding worker running indefinitely and holding its temporary
replication slot.
Fix by registering a before_shmem_exit callback when the decoding worker
is started. The callback calls stop_repack_decoding_worker(), which
terminates the worker and waits for it to shut down. Since
before_shmem_exit runs while shared memory is still attached,
WaitForBackgroundWorkerShutdown() is safe to call at this stage.
---
src/backend/commands/repack.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 860e2aecbe9..5e6494bf561 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -64,6 +64,7 @@
#include "pgstat.h"
#include "replication/logicalrelation.h"
#include "storage/bufmgr.h"
+#include "storage/ipc.h"
#include "storage/lmgr.h"
#include "storage/predicate.h"
#include "storage/proc.h"
@@ -211,6 +212,7 @@ static Oid determine_clustered_index(Relation rel, bool usingindex,
static void start_repack_decoding_worker(Oid relid);
static void stop_repack_decoding_worker(void);
+static void repack_decoding_worker_exit_cleanup(int code, Datum arg);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
static void ProcessRepackMessage(StringInfo msg);
@@ -3454,6 +3456,8 @@ start_repack_decoding_worker(Oid relid)
decoding_worker->seg = seg;
decoding_worker->error_mqh = mqh;
+ before_shmem_exit(repack_decoding_worker_exit_cleanup, (Datum) 0);
+
/*
* 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,
@@ -3477,6 +3481,17 @@ start_repack_decoding_worker(Oid relid)
ConditionVariableCancelSleep();
}
+/*
+ * before_shmem_exit callback to stop the repack decoding worker. Needed
+ * because SIGTERM (e.g., pg_terminate_backend) causes a FATAL exit, which
+ * bypasses PG_FINALLY blocks.
+ */
+static void
+repack_decoding_worker_exit_cleanup(int code, Datum arg)
+{
+ stop_repack_decoding_worker();
+}
+
/*
* Stop the decoding worker and cleanup the related resources.
*
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
2026-05-12 23:26 [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
@ 2026-05-17 20:46 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-05-18 00:34 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
1 sibling, 1 reply; 8+ messages in thread
From: Alvaro Herrera @ 2026-05-17 20:46 UTC (permalink / raw)
To: Baji Shaik <baji.pgdev@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
On 2026-May-12, Baji Shaik wrote:
> Root cause:
>
> pg_terminate_backend() causes ereport(FATAL) via ProcDiePending.
> FATAL exits bypass PG_FINALLY blocks, so stop_repack_decoding_worker()
> is never called. The decoding worker is left running.
>
> Fix:
>
> Register an on_proc_exit callback when the decoding worker starts.
I think a better fix for this is to use PG_ENSURE_ERROR_CLEANUP(). That
way we avoid leaving callbacks in place, which would not be great if the
same backend does a lot of REPACKs: after a dozen or so, it dies with
FATAL: out of before_shmem_exit slots
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
2026-05-12 23:26 [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
2026-05-17 20:46 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-05-18 00:34 ` Baji Shaik <baji.pgdev@gmail.com>
2026-05-19 18:45 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 8+ messages in thread
From: Baji Shaik @ 2026-05-18 00:34 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: pgsql-hackers@lists.postgresql.org
Hi Alvaro, Sami,
Thank you both for the feedback.
Sami, thanks for identifying the use-after-free with on_proc_exit
and providing the v2 patch with before_shmem_exit. That helped
narrow down the right approach.
On Sun, May 17, 2026 at 3:46 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:
> I think a better fix for this is to use PG_ENSURE_ERROR_CLEANUP(). That
> way we avoid leaving callbacks in place, which would not be great if the
> same backend does a lot of REPACKs: after a dozen or so, it dies with
>
> FATAL: out of before_shmem_exit slots
>
>
Alvaro, you're right that PG_ENSURE_ERROR_CLEANUP is the better
approach here. Attached is v3 which uses it instead of
before_shmem_exit or on_proc_exit.
Summary of the issues with v1 and v2:
- v1 (on_proc_exit): Crashes with assertions enabled because
memory contexts are already destroyed by the time on_proc_exit
callbacks run. The worker handle is clobbered with 0x7f
(CLOBBER_FREED_MEMORY).
- v2 (before_shmem_exit): Works correctly for a single REPACK,
but leaks a callback slot on each successful REPACK
(CONCURRENTLY). After ~15 REPACKs in the same session:
FATAL: out of before_shmem_exit slots.
v3 uses PG_ENSURE_ERROR_CLEANUP which:
- Handles both ERROR and FATAL exits
- Automatically cancels the callback on normal completion
(no slot leak)
- Runs before memory contexts are destroyed (no use-after-free)
The existing PG_TRY/PG_FINALLY block is replaced with
PG_ENSURE_ERROR_CLEANUP for the concurrent path, and a plain
rebuild_relation() call for the non-concurrent path.
Tested with --enable-cassert --enable-debug --enable-injection-points:
- All 245 regression tests pass (including cluster)
- All 8 injection point tests pass (including repack and repack_toast)
- pg_terminate_backend cleans up worker and slot without crash
- 20 REPACK (CONCURRENTLY) in same session completes without
slot exhaustion
I have not added a dedicated regression test for the
pg_terminate_backend scenario yet, but I can write one using
injection points if needed.
Thanks,
Baji Shaik
Attachments:
[application/octet-stream] v3-0001-Fix-REPACK-decoding-worker-not-cleaned-up-on-FATAL-exit.patch (3.5K, ../../CA+fm-RN2oL=mWXA6NLKNS5wrCXLML-z+O6euKW1ga2ZEYFVQ0Q@mail.gmail.com/3-v3-0001-Fix-REPACK-decoding-worker-not-cleaned-up-on-FATAL-exit.patch)
download | inline diff:
From b7ace6f7eab4a2c181427ac4a7719da62faf78ee Mon Sep 17 00:00:00 2001
From: Baji Shaik <baji.pgdev@gmail.com>
Date: Sun, 17 May 2026 23:32:42 +0000
Subject: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
When the launching backend of REPACK (CONCURRENTLY) is terminated via
pg_terminate_backend(), ProcDiePending causes ereport(FATAL) which
bypasses PG_FINALLY blocks. As a result, stop_repack_decoding_worker()
is never called, leaving the decoding worker running indefinitely and
holding its temporary replication slot.
Fix by using PG_ENSURE_ERROR_CLEANUP, which handles both ERROR and
FATAL exits. Unlike before_shmem_exit, this does not leak callback
slots when the same backend runs multiple REPACK (CONCURRENTLY)
commands, and unlike on_proc_exit, it runs before memory contexts
are destroyed so the worker handle is still valid.
---
src/backend/commands/repack.c | 42 +++++++++++++++++++++++------------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index fae88d6bb83..17eb9eb029c 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -64,6 +64,7 @@
#include "pgstat.h"
#include "replication/logicalrelation.h"
#include "storage/bufmgr.h"
+#include "storage/ipc.h"
#include "storage/lmgr.h"
#include "storage/predicate.h"
#include "storage/proc.h"
@@ -211,6 +212,7 @@ static Oid determine_clustered_index(Relation rel, bool usingindex,
static void start_repack_decoding_worker(Oid relid);
static void stop_repack_decoding_worker(void);
+static void repack_decoding_worker_cleanup_cb(int code, Datum arg);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
static void ProcessRepackMessage(StringInfo msg);
@@ -660,24 +662,25 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid,
TransferPredicateLocksToHeapRelation(OldHeap);
/* rebuild_relation does all the dirty work */
- PG_TRY();
- {
- rebuild_relation(OldHeap, index, verbose, ident_idx);
- }
- PG_FINALLY();
+ if (concurrent)
{
- if (concurrent)
+ /*
+ * Use PG_ENSURE_ERROR_CLEANUP so that the decoding worker is stopped
+ * on both ERROR and FATAL exits. PG_FINALLY only handles ERROR;
+ * FATAL (e.g. from pg_terminate_backend) would bypass it, leaving
+ * the worker running and holding its replication slot indefinitely.
+ */
+ PG_ENSURE_ERROR_CLEANUP(repack_decoding_worker_cleanup_cb, (Datum) 0);
{
- /*
- * Since during normal operation the worker was already asked to
- * exit, stopping it explicitly is especially important on ERROR.
- * However it still seems a good practice to make sure that the
- * worker never survives the REPACK command.
- */
- stop_repack_decoding_worker();
+ rebuild_relation(OldHeap, index, verbose, ident_idx);
}
+ PG_END_ENSURE_ERROR_CLEANUP(repack_decoding_worker_cleanup_cb, (Datum) 0);
+ stop_repack_decoding_worker();
+ }
+ else
+ {
+ rebuild_relation(OldHeap, index, verbose, ident_idx);
}
- PG_END_TRY();
/* rebuild_relation closes OldHeap, and index if valid */
@@ -3482,6 +3485,17 @@ start_repack_decoding_worker(Oid relid)
ConditionVariableCancelSleep();
}
+/*
+ * PG_ENSURE_ERROR_CLEANUP callback to stop the decoding worker.
+ * This ensures the worker is terminated on both ERROR and FATAL exits,
+ * unlike PG_FINALLY which only handles ERROR.
+ */
+static void
+repack_decoding_worker_cleanup_cb(int code, Datum arg)
+{
+ stop_repack_decoding_worker();
+}
+
/*
* Stop the decoding worker and cleanup the related resources.
*
--
2.50.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
2026-05-12 23:26 [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
2026-05-17 20:46 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
2026-05-18 00:34 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
@ 2026-05-19 18:45 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-05-20 07:54 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 8+ messages in thread
From: Alvaro Herrera @ 2026-05-19 18:45 UTC (permalink / raw)
To: Baji Shaik <baji.pgdev@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
On 2026-May-17, Baji Shaik wrote:
> v3 uses PG_ENSURE_ERROR_CLEANUP which:
> - Handles both ERROR and FATAL exits
> - Automatically cancels the callback on normal completion
> (no slot leak)
> - Runs before memory contexts are destroyed (no use-after-free)
Yeah, looks good. I have pushed it, with some comment wordsmithing and
other cosmetic changes.
While looking at it, I realized that I didn't like the way
stop_repack_decoding_worker() works, mainly because if there's no
handle, we leak everything else -- and the way we initialize things
means we leak the shared memory segment. This is maybe a rare case and
just a small memory leak, but it seems better to do it nicely. So
here's a followup patch that reworks that code. This also forced me to
understand more clearly what is going on, so I rewrote the comments.
> - 20 REPACK (CONCURRENTLY) in same session completes without
> slot exhaustion
FWIW I tested this by doing "repack (concurrently) foo \watch 0.1" and
letting it run for some time. I happened to notice that if I have two
psqls running, one with the above and the second with the equivalent for
table bar, when they run together, each runs more quickly than when only
one of them is running. I don't know what causes this; I suspect/assume
it's because the WAL messages for initial historic snapshot creation
from one gets the other running.
> I have not added a dedicated regression test for the
> pg_terminate_backend scenario yet, but I can write one using
> injection points if needed.
I don't feel a need for that.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Attachments:
[text/x-diff] 0001-Restructure-repack-worker-teardown.patch (4.6K, ../../agtNn6ZCmdI2KJFn@alvherre.pgsql/2-0001-Restructure-repack-worker-teardown.patch)
download | inline diff:
From db6f070e73f89c38f8a2bfafa10b0718ea51a57d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Mon, 18 May 2026 10:13:24 -0700
Subject: [PATCH] Restructure repack worker teardown
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The original code would leave a shared memory segment unreleased if we
fail partway through initialization. Change the shutdown order so that
we already free it.
Author: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/agtNn6ZCmdI2KJFn@alvherre.pgsql
---
src/backend/commands/repack.c | 67 ++++++++++++++++-------------------
1 file changed, 31 insertions(+), 36 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index bfc62c8f752..c9064d8fd13 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -3411,10 +3411,14 @@ start_repack_decoding_worker(Oid relid)
shm_mq_handle *mqh;
BackgroundWorker bgw;
+ decoding_worker = palloc0_object(DecodingWorker);
+
/* Setup shared memory. */
size = BUFFERALIGN(offsetof(DecodingWorkerShared, error_queue)) +
BUFFERALIGN(REPACK_ERROR_QUEUE_SIZE);
seg = dsm_create(size, 0);
+ decoding_worker->seg = seg;
+
shared = (DecodingWorkerShared *) dsm_segment_address(seg);
shared->initialized = false;
shared->lsn_upto = InvalidXLogRecPtr;
@@ -3454,14 +3458,12 @@ start_repack_decoding_worker(Oid relid)
bgw.bgw_main_arg = UInt32GetDatum(dsm_segment_handle(seg));
bgw.bgw_notify_pid = MyProcPid;
- decoding_worker = palloc0_object(DecodingWorker);
if (!RegisterDynamicBackgroundWorker(&bgw, &decoding_worker->handle))
ereport(ERROR,
errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
errmsg("out of background worker slots"),
errhint("You might need to increase \"%s\".", "max_worker_processes"));
- decoding_worker->seg = seg;
decoding_worker->error_mqh = mqh;
/*
@@ -3487,17 +3489,6 @@ start_repack_decoding_worker(Oid relid)
ConditionVariableCancelSleep();
}
-/*
- * PG_ENSURE_ERROR_CLEANUP callback to stop the decoding worker.
- * This ensures the worker is terminated on both ERROR and FATAL exits,
- * unlike PG_FINALLY which only handles ERROR.
- */
-static void
-repack_decoding_worker_cleanup_cb(int code, Datum arg)
-{
- stop_repack_decoding_worker();
-}
-
/*
* Stop the decoding worker and cleanup the related resources.
*
@@ -3508,39 +3499,43 @@ static void
stop_repack_decoding_worker(void)
{
BgwHandleStatus status;
+ dsm_segment *dsmseg;
- /* Haven't reached the worker startup? */
+ /* Nothing to do if no worker was set up. */
if (decoding_worker == NULL)
return;
- /* Could not register the worker? */
- if (decoding_worker->handle == NULL)
- return;
+ /* Terminate the worker process, if one is running. */
+ if (decoding_worker->handle != NULL)
+ {
+ TerminateBackgroundWorker(decoding_worker->handle);
+ /* The worker should really exit before the REPACK command does. */
+ HOLD_INTERRUPTS();
+ status = WaitForBackgroundWorkerShutdown(decoding_worker->handle);
+ RESUME_INTERRUPTS();
- TerminateBackgroundWorker(decoding_worker->handle);
- /* The worker should really exit before the REPACK command does. */
- HOLD_INTERRUPTS();
- status = WaitForBackgroundWorkerShutdown(decoding_worker->handle);
- RESUME_INTERRUPTS();
-
- if (status == BGWH_POSTMASTER_DIED)
- ereport(FATAL,
- errcode(ERRCODE_ADMIN_SHUTDOWN),
- errmsg("postmaster exited during REPACK command"));
-
- shm_mq_detach(decoding_worker->error_mqh);
+ if (status == BGWH_POSTMASTER_DIED)
+ ereport(FATAL,
+ errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("postmaster exited during REPACK command"));
+ }
/*
- * If we could not cancel the current sleep due to ERROR, do that before
- * we detach from the shared memory the condition variable is located in.
- * If we did not, the bgworker ERROR handling code would try and fail
- * badly.
+ * 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.
*/
- ConditionVariableCancelSleep();
-
- dsm_detach(decoding_worker->seg);
+ dsmseg = decoding_worker->seg;
pfree(decoding_worker);
decoding_worker = NULL;
+
+ /* We must also cancel the current sleep, if one is still set up */
+ ConditionVariableCancelSleep();
+
+ if (dsmseg != NULL)
+ dsm_detach(dsmseg);
}
/* stop_repack_decoding_worker, wrapped as a before_shmem_exit callback */
--
2.47.3
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
2026-05-12 23:26 [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
2026-05-17 20:46 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
2026-05-18 00:34 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
2026-05-19 18:45 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-05-20 07:54 ` Antonin Houska <ah@cybertec.at>
2026-05-20 20:13 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 8+ messages in thread
From: Antonin Houska @ 2026-05-20 07:54 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: Baji Shaik <baji.pgdev@gmail.com>; pgsql-hackers@lists.postgresql.org
Alvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2026-May-17, Baji Shaik wrote:
>
> > v3 uses PG_ENSURE_ERROR_CLEANUP which:
> > - Handles both ERROR and FATAL exits
> > - Automatically cancels the callback on normal completion
> > (no slot leak)
> > - Runs before memory contexts are destroyed (no use-after-free)
>
> Yeah, looks good. I have pushed it, with some comment wordsmithing and
> other cosmetic changes.
>
> While looking at it, I realized that I didn't like the way
> stop_repack_decoding_worker() works, mainly because if there's no
> handle, we leak everything else -- and the way we initialize things
> means we leak the shared memory segment. This is maybe a rare case and
> just a small memory leak, but it seems better to do it nicely. So
> here's a followup patch that reworks that code. This also forced me to
> understand more clearly what is going on, so I rewrote the comments.
The call of shm_mq_detach() got lost, or do you rely on dsm_detach() to call
shm_mq_detach_callback() ? The latter does not free ->mqh_buffer. Since each
REPACK runs in a separate transaction, I wouldn't consider that a leak, but I
still think that explicit call of shm_mq_detach() makes the code a bit easier
to read (i.e. no need for the developer to check if the detaching happens
automatically).
/*
- * If we could not cancel the current sleep due to ERROR, do that before
- * we detach from the shared memory the condition variable is located in.
- * If we did not, the bgworker ERROR handling code would try and fail
- * badly.
+ * 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.
*/
- ConditionVariableCancelSleep();
-
- dsm_detach(decoding_worker->seg);
+ dsmseg = decoding_worker->seg;
pfree(decoding_worker);
decoding_worker = NULL;
+
+ /* We must also cancel the current sleep, if one is still set up */
+ ConditionVariableCancelSleep();
+
+ if (dsmseg != NULL)
+ dsm_detach(dsmseg);
I suppose the reason for the assertion failure was reading from the queue
after the backend had detached from it? Thanks for fixing that.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
2026-05-12 23:26 [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
2026-05-17 20:46 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
2026-05-18 00:34 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
2026-05-19 18:45 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
2026-05-20 07:54 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Antonin Houska <ah@cybertec.at>
@ 2026-05-20 20:13 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-05-26 15:26 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 8+ messages in thread
From: Alvaro Herrera @ 2026-05-20 20:13 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Baji Shaik <baji.pgdev@gmail.com>; pgsql-hackers@lists.postgresql.org
On 2026-May-20, Antonin Houska wrote:
> Alvaro Herrera <alvherre@kurilemu.de> wrote:
> The call of shm_mq_detach() got lost, or do you rely on dsm_detach() to call
> shm_mq_detach_callback() ? The latter does not free ->mqh_buffer.
Hmm, this I think this is a code documentation bug then, because the
comment for shm_mq_attach() says
* If seg != NULL, the queue will be automatically detached when that dynamic
* shared memory segment is detached.
I think it's strange, or buggy even, to say that "the queue is
automatically detached", but that you still have to call dsm_mq_detach()
afterwards.
I can put back the shm_mq_detach() call, of course.
> + * 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.
> I suppose the reason for the assertion failure was reading from the queue
> after the backend had detached from it? Thanks for fixing that.
Yeah, that's exactly what happened.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La vida es para el que se aventura"
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit
2026-05-12 23:26 [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
2026-05-17 20:46 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
2026-05-18 00:34 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
2026-05-19 18:45 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
2026-05-20 07:54 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Antonin Houska <ah@cybertec.at>
2026-05-20 20:13 ` Re: [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-05-26 15:26 ` Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 0 replies; 8+ messages in thread
From: Alvaro Herrera @ 2026-05-26 15:26 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Baji Shaik <baji.pgdev@gmail.com>; pgsql-hackers@lists.postgresql.org
On 2026-May-20, Alvaro Herrera wrote:
> On 2026-May-20, Antonin Houska wrote:
>
> > The call of shm_mq_detach() got lost, or do you rely on dsm_detach() to call
> > shm_mq_detach_callback() ? The latter does not free ->mqh_buffer.
> I can put back the shm_mq_detach() call, of course.
I did that and pushed.
Many thanks!
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-05-26 15:26 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-12 23:26 [PATCH] Fix REPACK decoding worker not cleaned up on FATAL exit Baji Shaik <baji.pgdev@gmail.com>
2026-05-13 03:45 ` Sami Imseih <samimseih@gmail.com>
2026-05-17 20:46 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-05-18 00:34 ` Baji Shaik <baji.pgdev@gmail.com>
2026-05-19 18:45 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-05-20 07:54 ` Antonin Houska <ah@cybertec.at>
2026-05-20 20:13 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-05-26 15:26 ` Alvaro Herrera <alvherre@kurilemu.de>
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