First seen: 2026-05-12 23:26:23+00:00 · Messages: 8 · Participants: 4
Álvaro Herrera confirms he incorporated the reviewer's feedback (re-adding explicit shm_mq_detach() call) and pushed the follow-up cleanup patch.
This is a simple acknowledgment message with no new technical content beyond confirming the commit.
The thread has two new messages that constitute a focused code review exchange between an unnamed reviewer (ah@cybertec.at) and Álvaro Herrera regarding the follow-up cleanup patch.
shm_mq_detach() omission: The reviewer notes that the reworked stop_repack_decoding_worker() no longer explicitly calls shm_mq_detach(), instead relying on dsm_detach() to trigger shm_mq_detach_callback() automatically. The reviewer points out this implicit path does NOT free ->mqh_buffer, and while not a true leak (each REPACK runs in a separate transaction so the memory context is destroyed anyway), the explicit call improves code readability.
Álvaro's response — possible documentation bug in shm_mq_attach(): Álvaro identifies what he considers a documentation inconsistency in core infrastructure: shm_mq_attach()'s comment claims the queue is "automatically detached" when the DSM segment is detached, yet the automatic detach callback doesn't fully clean up (doesn't free mqh_buffer). He characterizes this as "strange, or buggy even" — suggesting the core shm_mq API documentation may need correction. He agrees to put back the explicit shm_mq_detach() call.
Assertion failure root cause confirmed: The reviewer confirms understanding that the assertion failure in the error path was caused by attempting to read from the shared memory queue after the backend had already detached from it. Álvaro confirms: "Yeah, that's exactly what happened." This validates the reordering in the follow-up patch where decoding_worker is set to NULL before dsm_detach() is called, preventing ProcessRepackMessages from trying to read a detached queue.
This is a minor code review exchange with no architectural changes or position shifts. The substantive fix is already committed. The discussion refines the follow-up patch's handling of shm_mq_detach() and surfaces a potential documentation issue in PostgreSQL's shared memory queue API.
Álvaro Herrera has committed the v3 patch (PG_ENSURE_ERROR_CLEANUP approach) with minor cosmetic/comment changes. The core bug fix is now in the tree.
Álvaro identified an additional issue while reviewing the committed code and produced a follow-up patch:
Problem: stop_repack_decoding_worker() has a logic flaw where if there's no BackgroundWorkerHandle (e.g., worker registration failed or handle was never set), the function returns early and leaks the shared memory segment allocated for communication with the worker. The initialization order means the DSM segment can exist without a valid handle.
Fix: Reworks the function to always clean up all resources (particularly the DSM segment) regardless of whether the handle is valid. This also required rewriting comments for clarity about what each piece of state represents and when it's valid.
Álvaro notes an unexpected performance behavior: two concurrent REPACK (CONCURRENTLY) operations on different tables each run faster together than individually. His hypothesis is that WAL messages for initial historic snapshot creation from one operation unblock the other's decoding worker, reducing wait time. This is an interesting but unrelated observation about the WAL-based synchronization between logical decoding workers.
Álvaro explicitly declined Baji's offer to write an automated regression test using injection points ("I don't feel a need for that"), suggesting confidence that the fix is straightforward enough to not warrant dedicated test infrastructure.
The thread has converged on a third approach after both v1 and v2 were found to have fundamental flaws. Álvaro Herrera identified a critical scalability problem with the before_shmem_exit() approach (v2), and Baji Shaik produced a v3 patch incorporating his suggestion.
Sami Imseih apparently produced a v2 patch (between analyses) using before_shmem_exit() which fixed the segfault. However, Álvaro identified that before_shmem_exit callbacks are never unregistered on successful completion — PostgreSQL has a fixed-size array of slots (typically ~20). Each successful REPACK (CONCURRENTLY) in the same session would consume one slot permanently. After ~15 REPACKs: FATAL: out of before_shmem_exit slots. This is a session-lifetime resource exhaustion bug that makes v2 unsuitable for production use where sessions are reused.
PG_ENSURE_ERROR_CLEANUP is a macro that:
PG_END_ENSURE_ERROR_CLEANUP)BackgroundWorkerHandle is still valid)This solves all three problems simultaneously:
on_proc_exit)before_shmem_exit)PG_FINALLY which only handles ERROR)The macro is specifically designed for this pattern: protecting a resource that needs cleanup on abnormal exit but should NOT be cleaned up on normal completion. It's used elsewhere in PostgreSQL for similar "launch something, wait for it, clean up if interrupted" patterns. The existing PG_TRY/PG_FINALLY block in the REPACK concurrent path is replaced entirely.
Baji confirms the v1 crash mechanism: with CLOBBER_FREED_MEMORY enabled (cassert builds), the BackgroundWorkerHandle is overwritten with 0x7f bytes after its memory context is freed. The on_proc_exit callback then dereferences this clobbered pointer. This explains why the crash was reproducible in debug builds but might have appeared to "work" (use-after-free with stale but not yet overwritten memory) in production builds — making it a latent data corruption risk.
v3 passes full regression suite including cassert/debug builds. The specific scenario (pg_terminate_backend during REPACK CONCURRENTLY) is manually verified to clean up both the worker and slot. No dedicated automated regression test yet, though Baji offers to write one using injection points.