Hi, On Tue, Jun 16, 2026 at 4:28 PM Masahiko Sawada <[email protected]> wrote: > > I've reviewed the v6 patch, and here are some comments:
Thanks Satya for the off-list discussion on this. Thanks Sawada-san and others for reviewing it. I read the issue, patches and comments so far and here's my take on it. When an error reaches the top-level sigsetjmp in PostgresMain(), it releases the acquired replication slot and cleans up all the session-level temp slots. However, when a subtransaction (subxact) catches the error in its own PG try-catch block without re-throwing it, that never happens, and the acquired slot can be left behind, leaving the session-level MyReplicationSlot pointing to an unreleased slot. In assert builds this causes a failure. In release builds it silently leaves a dangling slot, retaining WAL and blocking vacuum due to catalog_xmin. Any SQL-callable replication slot function can hit this (except pg_sync_replication_slots(), which already releases the slot on error via PG_ENSURE_ERROR_CLEANUP()), especially when called inside a PL/pgSQL, PL/Perl, PL/Python or PL/Tcl exception block, which under the hood is implemented as subxact with a PG try-catch block in more or less the same way. This happens on all supported branches, back to PG14. Here's the flow in each case: Transaction: PostgresMain()'s sigsetjmp -> slot function. An error in the slot function reaches PostgresMain()'s sigsetjmp, which releases the slot. Subxact: PostgresMain()'s sigsetjmp -> exec_stmts's sigsetjmp -> slot function. An error in the slot function inside the subxact gets to exec_stmts's sigsetjmp, which doesn't re-throw the error and so never reaches PostgresMain()'s sigsetjmp, leaving the slot behind. To fix this, we need to ensure the slot acquired in the subxact also gets released in the error paths. There are a couple of ways of fixing it. See below. > Maybe pg_sync_replication_slots() has the same problem if it's called > inside an exception block? No, it already wraps the code that runs with the slot acquired in PG_ENSURE_ERROR_CLEANUP(), which releases the slot on error. I verified this locally with an injection point. > The patch adds PG_TRY()/PG_CATCH() to each replication slot function, > but there is no comment explaining why we need them even though we > call ReplicationSlotRelease() and ReplicationSlotCleanup() in error > paths. Also, while probably the proposed idea works for back branches, > I guess we might want to consider more comprehensive approach for HEAD > to deal with this issue as it seems to me very error-prone, > especially when adding a new replication slot function. This approach wraps each SQL-callable slot function in its own PG try-catch that releases the slot on error and re-throws, as proposed in the earlier patches. The flow then becomes PostgresMain()'s sigsetjmp -> exec_stmts's sigsetjmp -> slot function's sigsetjmp -> slot function. An error gets to the slot function's own sigsetjmp first, which releases the slot and re-throws, so by the time it reaches exec_stmts's sigsetjmp the slot is already released. Although it looks simple and keeps the changes localized to the slot functions, it has to be repeated in every such function and any future one, so the change is spread across many callsites, adding a PG try-catch to each. That gets error-prone as new slot functions are added and is a bit hard to maintain long-term. > This problem stems from the fact that when replication slots are used > within an exception block, we don't reach the error path even if an > error occurs, and we cannot simply call ReplicationSlotRelease() > during subtransaction abort as we need to start and abort transaction > while holding a slot. But I guess that we can release the slot when > aborting the subtransaction or above (sub)transaction where we > created/acquired the slot. I've drafted the patch for this idea and > confirmed it passes the regression tests, I still need to verify its > feasibility though. Thanks for the POC, and the attached patch builds on it. It releases the slot on subxact abort, at the subxact where it was acquired, instead of in each function. It records the subxact id that acquired the slot and, at every subxact end, checks whether that subxact is the one ending. On abort of the acquiring subxact it releases the slot. On commit it hands the id to the parent, so the slot is still released if the parent, or an ancestor, later aborts. A subxact id is used rather than a nesting level, since levels are reused. It does touch the subxact abort path, which is shared by all transactions, so it needs some care to not release the slot that logical decoding holds. As you noted, decoding starts and aborts an internal transaction (or subxact) per decoded transaction while holding the slot. That always happens below the acquiring subxact, so matching on the acquiring subxact id leaves those alone. > + /* XXX need to call ReplicationSlotCleanup() as well? */ Releasing a temporary slot does not drop it, so it would keep retaining WAL and blocking vacuum until session end. So on abort, after releasing the held slot, the patch also drops the session's temporary slots the same way the top-level path does. This runs only when the aborting subxact held a slot, so a caught error that held no slot does not drop them, unlike a top-level error which always does. I left an XXX comment in the patch with more details on this. Please have a look at the attached v7 patch. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
v7-0001-Fix-replication-slot-leak-on-error-caught-in-a-su.patch
Description: Binary data
