Hi, On Tue, Sep 15, 2026 at 5:13 PM Masahiko Sawada <[email protected]> wrote: > > I looked at the back-branch ones and I think they have a problem that > the HEAD patch doesn't have. The back branches return early on subxact > commit: > > + if (isCommit) > + return; > > So once the subxact that acquired the slot commits, > MyReplicationSlotSubId keeps the id of a subxact that is already gone. > Subxact ids restart at TopSubTransactionId in every transaction since > StartTransaction() resets currentSubTransactionId, so the same id > comes around again.It's not a problem for the core use cases, but if > there is an external SQL function that keeps the slot when the > transaction ends, that stale id can match a completely unrelated > subxact in a later transaction and we release a slot that subxact > never acquired. > > What bothers me is that this pattern works today on all branches. > While I guess it's not a good programming practice, we don't restrict > such use cases. So I think it's not a case of not supporting that > usage, it's a behavior change we would be introducing in a minor > release. > > That makes me want to reconsider how we split the patches. IIUC the > handoff mechanism that the master patch implements is to (1) keep > MyReplicationSlotSubId from going stale and (2) give the slot a new > guarantee, that the slot is released if an ancestor subxact aborts, > which nothing does today. (2) is the part that broadens what an > extension can do whereas (1) is just cleaning up after the variable we > added. I think we can fix the reported problem only with (1) even > without (2). So I guess it would be cleaner to do (1) for all > branches, and do (2) only for master. As for (1), we can have a > function like AtEOXact_ReplicationSlot() just clearing > MyReplicationSlotSubId. For (2), we can prepare a separate patch that > implements the handoff mechanism (possibly with a WARNING or DEBUG > message) with the regression tests, if we want to support these cases. > > It seems confusing and I might be too pessimistic as this is all about > hypothetical cases that might not exist, but I'd like to keep the > back-branch fix to the smallest thing that fixes only the reported > problem while not changing other current behaviors.
Thanks for taking a look at it. Here is my thinking on this. Within a transaction, subtransaction ids are not reset or reused, so MyReplicationSlotSubId isn't stale. It is the id of the subtransaction that owns the slot (ownership stays with the subtransaction that acquired it until the slot is released, and the id gets reset at that point). And if the owning subtransaction aborts, the slot is released, which is what protects against the issue reported in this thread (leftover slot or assertion failure). Without the handoff on commit, the slot stops being protected at the first subtransaction boundary. If the owning subtransaction commits while still holding the slot, whether it holds the slot intentionally or unknowingly forgets to release the slot, MyReplicationSlotSubId keeps the id of an owner that is gone, so no later subtransaction in the transaction can match it. If the parent subtransaction then aborts, it is not the owner, so the slot is not released even though the work it was acquired for is being rolled back, and that brings back the issue reported in this thread. With the handoff on commit, the parent becomes the owner. If the parent aborts, it is now the owner, so the slot is released, and the protection against the issue reported in this thread still holds. If the parent commits, ownership moves up again, and it eventually reaches TopSubTransactionId, which is the top-level transaction's own id and is never assigned to a subtransaction, so no owner id is left behind that could match an unrelated subtransaction later. Therefore, handing the slot off to the parent on commit looks correct to me on all branches, even when the slot is carried across a subtransaction boundary, because ownership moves to a subtransaction that is still in progress and the slot stays protected from the issue reported in this thread for as long as it is held. And I am okay with not emitting any warning on any branch, including HEAD. For the legitimate usage, a function that intentionally holds the slot across subtransaction boundaries, the warning would be reporting correct code as a problem, and that is as wrong on HEAD as it is in the back branches. With no warning on any branch, AtEOSubXact_ReplicationSlot() ends up identical on all branches, so I don't think we need to split this into a back-branch patch and a separate HEAD patch. On adding AtEOXact_ReplicationSlot(), I don't think it is needed for correctness, for the reason above, but I have no objection to adding it on HEAD, similar to what AtEOXact_LargeObject() and AtEOXact_Files() do today. I previously tested the handoff in the commit path, by making pg_replication_slot_advance() return while still holding the slot and running it through nested PL/pgSQL exception blocks: https://postgr.es/m/calj2acud_k5zbgxd3ebymhmoujx91fq+ailed8hsuc6xnyv...@mail.gmail.com. If we want to add this test, I think I can add an injection point that returns before the slot release (similar to the skip-log-running-xacts test), and add the test on all the possible back branches. Thoughts? -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
