On Fri, Jul 31, 2026 at 12:43 AM Bharath Rupireddy <[email protected]> wrote: > > 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. >
Hi Bharath, Sawana-San, I definitely like this approach better. I'll review it and verify it in more depth soon. thanks Shveta
