From 780d14009f21730ff1fa728d3ed7e146452a3ebb Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Wed, 9 Sep 2026 20:44:27 +0000 Subject: [PATCH v16] Fix replication slot leak on error caught in a subtransaction. The SQL-callable replication slot functions, such as pg_replication_slot_advance(), acquire a slot and release it before returning. When one of them throws an error, the slot is normally released by the top-level error handler (the sigsetjmp block in PostgresMain()). However, PL/pgSQL, PL/Perl, PL/Python and PL/Tcl run their error-handling block in an internal subtransaction. When an error is raised there and a handler matches, they abort the subtransaction and catch the error without re-throwing it, so the top-level handler is never reached. Nothing on the (sub)transaction abort path releases the slot either, so it is left acquired after the caught error. As a result, the next slot operation in the session fails an assertion, or in a non-assertion build silently overwrites the reference and leaks the slot, which keeps holding back WAL removal and the catalog xmin (blocking vacuum) and can no longer be acquired. An error that does reach the top-level handler, such as a plain ROLLBACK TO SAVEPOINT or a PL/pgSQL exception handler whose conditions do not match and which then re-throws, is unaffected. Fix this by recording the subtransaction that acquires the slot and releasing the slot when that subtransaction aborts, the same way other subtransaction-scoped resources are released, for example in AtEOSubXact_LargeObject() and AtEOSubXact_Files(). The slot cannot simply be released on every subtransaction abort, because logical decoding starts and aborts an internal transaction or subtransaction for each decoded transaction while holding the slot; those internal aborts are nested below the acquiring subtransaction and so are left alone. Note that the subtransaction abort path only releases the slot; it does not drop the session's temporary slots the way the top-level error handler does. An error caught within a subtransaction is normally meant to be handled so the session continues, unlike a top-level error, so temporary slots are left in place. This matches the behavior that predates this fix, where a temporary slot is dropped only at session end or on a top-level error, and is simpler to reason about. The documentation is adjusted accordingly. Backpatch to all supported branches, as the leak is reachable from SQL. The back branches only release the slot on subtransaction abort, not on commit: such a commit can't be reproduced and may have no legitimate use, so only PG20+ warns and hands the slot to the parent in that case. Reported-by: Satya Narlapuram Author: Bharath Rupireddy Co-authored-by: Satya Narlapuram Co-authored-by: Masahiko Sawada Reviewed-by: Fujii Masao Reviewed-by: shveta malik Reviewed-by: Hou Zhijie Reviewed-by: Kyotaro Horiguchi Reviewed-by: Ashutosh Sharma Reviewed-by: Amit Kapila Discussion: https://postgr.es/m/CAHg+QDeuf9tCq3ce=kgFMJP0m=PZC+wi6B=yS+7V0vNXjLS31w@mail.gmail.com Backpatch-through: 14 --- contrib/test_decoding/expected/slot.out | 61 +++++++++++++++++++++++ contrib/test_decoding/sql/slot.sql | 32 ++++++++++++ doc/src/sgml/protocol.sgml | 5 +- doc/src/sgml/system-views.sgml | 5 +- src/backend/access/transam/xact.c | 4 ++ src/backend/replication/slot.c | 65 +++++++++++++++++++++++++ src/backend/tcop/postgres.c | 4 +- src/include/replication/slot.h | 2 + 8 files changed, 175 insertions(+), 3 deletions(-) diff --git a/contrib/test_decoding/expected/slot.out b/contrib/test_decoding/expected/slot.out index 6faeebfd8ab..02cf80f9876 100644 --- a/contrib/test_decoding/expected/slot.out +++ b/contrib/test_decoding/expected/slot.out @@ -409,3 +409,64 @@ SELECT pg_drop_replication_slot('copied_slot2_notemp'); (1 row) +-- A slot function that errors out must still release the slot, otherwise the +-- next slot operation in the session fails an assertion or leaks the slot. +-- Advancing a freshly created slot to a low LSN always errors. +SELECT 'init' FROM pg_create_logical_replication_slot('regress_subxact_slot', 'test_decoding'); + ?column? +---------- + init +(1 row) + +-- Error raised inside a PL/pgSQL block with an EXCEPTION clause is caught in a +-- subtransaction; the slot must still be released. +DO $$ +BEGIN + PERFORM pg_replication_slot_advance('regress_subxact_slot', '0/1'); +EXCEPTION WHEN OTHERS THEN + RAISE NOTICE 'caught SQLSTATE %', SQLSTATE; +END $$; +NOTICE: caught SQLSTATE 55000 +SELECT active FROM pg_replication_slots WHERE slot_name = 'regress_subxact_slot'; + active +-------- + f +(1 row) + +SELECT count(*) >= 0 AS peek_ok + FROM pg_logical_slot_peek_changes('regress_subxact_slot', NULL, NULL); + peek_ok +--------- + t +(1 row) + +-- The EXCEPTION clause does not match, so the error is not caught, but the +-- slot is still released. Show only the SQLSTATE for stable output. +\set VERBOSITY sqlstate +DO $$ +BEGIN + PERFORM pg_replication_slot_advance('regress_subxact_slot', '0/1'); +EXCEPTION WHEN division_by_zero THEN + RAISE NOTICE 'unreachable'; +END $$; +ERROR: 55000 +\set VERBOSITY default +SELECT active FROM pg_replication_slots WHERE slot_name = 'regress_subxact_slot'; + active +-------- + f +(1 row) + +SELECT count(*) >= 0 AS peek_ok + FROM pg_logical_slot_peek_changes('regress_subxact_slot', NULL, NULL); + peek_ok +--------- + t +(1 row) + +SELECT pg_drop_replication_slot('regress_subxact_slot'); + pg_drop_replication_slot +-------------------------- + +(1 row) + diff --git a/contrib/test_decoding/sql/slot.sql b/contrib/test_decoding/sql/slot.sql index f4d2b0cd557..b096550b9fe 100644 --- a/contrib/test_decoding/sql/slot.sql +++ b/contrib/test_decoding/sql/slot.sql @@ -179,3 +179,35 @@ ORDER BY o.slot_name, c.slot_name; SELECT pg_drop_replication_slot('orig_slot2'); SELECT pg_drop_replication_slot('copied_slot2_no_change'); SELECT pg_drop_replication_slot('copied_slot2_notemp'); + +-- A slot function that errors out must still release the slot, otherwise the +-- next slot operation in the session fails an assertion or leaks the slot. +-- Advancing a freshly created slot to a low LSN always errors. +SELECT 'init' FROM pg_create_logical_replication_slot('regress_subxact_slot', 'test_decoding'); + +-- Error raised inside a PL/pgSQL block with an EXCEPTION clause is caught in a +-- subtransaction; the slot must still be released. +DO $$ +BEGIN + PERFORM pg_replication_slot_advance('regress_subxact_slot', '0/1'); +EXCEPTION WHEN OTHERS THEN + RAISE NOTICE 'caught SQLSTATE %', SQLSTATE; +END $$; +SELECT active FROM pg_replication_slots WHERE slot_name = 'regress_subxact_slot'; +SELECT count(*) >= 0 AS peek_ok + FROM pg_logical_slot_peek_changes('regress_subxact_slot', NULL, NULL); + +-- The EXCEPTION clause does not match, so the error is not caught, but the +-- slot is still released. Show only the SQLSTATE for stable output. +\set VERBOSITY sqlstate +DO $$ +BEGIN + PERFORM pg_replication_slot_advance('regress_subxact_slot', '0/1'); +EXCEPTION WHEN division_by_zero THEN + RAISE NOTICE 'unreachable'; +END $$; +\set VERBOSITY default +SELECT active FROM pg_replication_slots WHERE slot_name = 'regress_subxact_slot'; +SELECT count(*) >= 0 AS peek_ok + FROM pg_logical_slot_peek_changes('regress_subxact_slot', NULL, NULL); +SELECT pg_drop_replication_slot('regress_subxact_slot'); diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index e030b55186c..aae186cf696 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -2036,7 +2036,10 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;" Specify that this replication slot is a temporary one. Temporary slots are not saved to disk and are automatically dropped on error - or when the session has finished. + or when the session has finished. An error raised and caught in a + subtransaction, for example by a + PL/pgSQL exception block, does not drop + them. diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml index 2e838fb61d3..89d55a41b96 100644 --- a/doc/src/sgml/system-views.sgml +++ b/doc/src/sgml/system-views.sgml @@ -2312,7 +2312,10 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx True if this is a temporary replication slot. Temporary slots are not saved to disk and are automatically dropped on error or when - the session has finished. + the session has finished. An error raised and caught in a + subtransaction, for example by a + PL/pgSQL exception block, does not drop + them. diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index a747b708d61..0986734b4c6 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -4979,6 +4979,8 @@ CommitSubTransaction(void) s->parent->curTransactionOwner); AtEOSubXact_LargeObject(true, s->subTransactionId, s->parent->subTransactionId); + AtEOSubXact_ReplicationSlot(true, s->subTransactionId, + s->parent->subTransactionId); AtSubCommit_Notify(); CallSubXactCallbacks(SUBXACT_EVENT_COMMIT_SUB, s->subTransactionId, @@ -5147,6 +5149,8 @@ AbortSubTransaction(void) s->parent->curTransactionOwner); AtEOSubXact_LargeObject(false, s->subTransactionId, s->parent->subTransactionId); + AtEOSubXact_ReplicationSlot(false, s->subTransactionId, + s->parent->subTransactionId); AtSubAbort_Notify(); /* Advertise the fact that we aborted in pg_xact. */ diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 6e3a561691a..0f43b0e79f0 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -50,6 +50,7 @@ #include "storage/proc.h" #include "storage/procarray.h" #include "utils/builtins.h" +#include "utils/snapmgr.h" /* * Replication slot on-disk data structure. @@ -96,6 +97,13 @@ ReplicationSlotCtlData *ReplicationSlotCtl = NULL; /* My backend's replication slot in the shared memory array */ ReplicationSlot *MyReplicationSlot = NULL; +/* + * Subxact that acquired MyReplicationSlot, or invalid if none is held + * or it was acquired with no transaction in progress (as a walsender does). + * Used to release the slot when that subxact aborts. + */ +static SubTransactionId MyReplicationSlotSubId = InvalidSubTransactionId; + /* GUCs */ int max_replication_slots = 0; /* the maximum number of replication * slots */ @@ -371,6 +379,7 @@ ReplicationSlotCreate(const char *name, bool db_specific, slot->active_pid = MyProcPid; SpinLockRelease(&slot->mutex); MyReplicationSlot = slot; + MyReplicationSlotSubId = GetCurrentSubTransactionId(); LWLockRelease(ReplicationSlotControlLock); @@ -555,6 +564,7 @@ retry: /* We made this slot active, so it's ours now. */ MyReplicationSlot = s; + MyReplicationSlotSubId = GetCurrentSubTransactionId(); /* * The call to pgstat_acquire_replslot() protects against stats for a @@ -624,6 +634,60 @@ ReplicationSlotRelease(void) MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING; ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; LWLockRelease(ProcArrayLock); + + /* The slot is no longer acquired in any subxact. */ + MyReplicationSlotSubId = InvalidSubTransactionId; +} + +/* + * At subxact end, release MyReplicationSlot if it was acquired in this + * subxact and the subxact aborts. + */ +void +AtEOSubXact_ReplicationSlot(bool isCommit, SubTransactionId mySubid, + SubTransactionId parentSubid) +{ + /* Nothing to do unless the slot was acquired in this subxact. */ + if (MyReplicationSlotSubId != mySubid) + return; + + /* + * Do nothing on commit. No core code path leaves a slot held past the + * commit of the subxact that acquired it, and we don't know whether an + * extension might. The back branches leave the slot alone here and + * release it only on abort. PG20 and later warn and hand the slot to the + * parent. + */ + if (isCommit) + return; + + /* + * We must not get here while decoding is running. Decoding starts and + * aborts an internal (sub)transaction while holding the slot, for each + * decoded transaction (ReorderBufferProcessTXN()) and when executing + * invalidations (ReorderBufferImmediateInvalidation()). However, those + * subtransactions are always nested below the one that acquired the slot, + * so their subtransaction ids are deeper and do not match here. Decoding + * also runs with a historic snapshot set up, so assert that it is not. + */ + Assert(!HistoricSnapshotActive()); + + /* + * The aborting subxact is the one that acquired the slot, so the slot is + * still held and must be released. MyReplicationSlotSubId is set only + * when a slot is held and cleared when it is released, so a matching + * subxact id means the slot is ours. + * + * We only release the slot here and do not drop the session's temporary + * slots, unlike the top-level error handler in PostgresMain(). An error + * caught within a subtransaction, for example by a PL/pgSQL exception + * block, is normally meant to be handled so the session carries on, + * unlike a top-level error, so a temporary slot is left in place. That + * matches the temporary slot behavior that predates this callback and is + * simpler to reason about; the slot lives on until the session ends or a + * top-level error occurs, as documented. + */ + ReplicationSlotRelease(); } /* @@ -689,6 +753,7 @@ ReplicationSlotDropAcquired(void) /* slot isn't acquired anymore */ MyReplicationSlot = NULL; + MyReplicationSlotSubId = InvalidSubTransactionId; ReplicationSlotDropPtr(slot); } diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 2ae682e2221..76e7a2eae0a 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -4405,7 +4405,9 @@ PostgresMain(const char *dbname, const char *username) * need to be able to start and abort transactions while having a slot * acquired. But we never need to hold them across top level errors, * so releasing here is fine. There also is a before_shmem_exit() - * callback ensuring correct cleanup on FATAL errors. + * callback for FATAL errors, and AtEOSubXact_ReplicationSlot() for an + * error caught in a subtransaction. The latter only releases the slot + * and, unlike here, does not drop the session's temporary slots. */ if (MyReplicationSlot != NULL) ReplicationSlotRelease(); diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index 51dfb740cfa..76ea0ae84fb 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -202,6 +202,8 @@ extern void ReplicationSlotDrop(const char *name, bool nowait); extern void ReplicationSlotAcquire(const char *name, bool nowait); extern void ReplicationSlotRelease(void); extern void ReplicationSlotCleanup(void); +extern void AtEOSubXact_ReplicationSlot(bool isCommit, SubTransactionId mySubid, + SubTransactionId parentSubid); extern void ReplicationSlotSave(void); extern void ReplicationSlotMarkDirty(void); -- 2.47.3