Hi, On Fri, Aug 28, 2026 at 3:49 PM Masahiko Sawada <[email protected]> wrote: > > On Thu, Aug 27, 2026 at 9:19 PM shveta malik <[email protected]> wrote: > > > > On Thu, Aug 27, 2026 at 6:00 PM Amit Kapila <[email protected]> wrote: > > > > > > On Thu, Aug 27, 2026 at 11:58 AM Masahiko Sawada <[email protected]> > > > wrote: > > > > > > > > On Mon, Aug 24, 2026 at 3:29 PM Bharath Rupireddy > > > > <[email protected]> wrote: > > > > > > > > > > > > > > > In short, having just the slot release in the subxact path gives the > > > > > same error behavior, is simple to reason about, and fixes the crash > > > > > reported in this thread. > > > > > > > > One thing I'm a bit concerned about is that this would be the first > > > > caller to invoke ReplicationSlotRelease() from inside the transaction > > > > machinery. > > > > > > > > > > True, but OTOH, won't we already clean up resources not directly > > > associated with subxact in AtEOSubXact_LargeObject() or > > > AtEOSubXact_Files()? I don't see any problem as far as the current > > > pattern of usage for slots. > > > > I agree. > > > > > The new restriction this patch will add is > > > "a slot acquired in a subxact does not survive that subxact being > > > unwound." which should be okay because of its similarity with > > > top-level xact behavior. I feel if possible we should restrict such > > > usage explicitly in code in some way rather than one finding out this > > > as a surprise. > > > > > > * > > > An error raised and caught in a > > > + subtransaction, for example by a > > > + <application>PL/pgSQL</application> exception block, does not > > > drop > > > + them. > > > > > > Based on above, something like below won't clean up temp slots and end > > > up holding xmin. > > > DO $$ BEGIN > > > PERFORM pg_create_logical_replication_slot('s', 'nonexistent_plugin', > > > true); > > > EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLERRM; > > > END $$; > > > > Well, on rethinking, I feel that if we encounter an error while > > creating a slot, whether persistent or temporary, the slot should be > > dropped right there. > > > > This already works correctly for persistent slots: by the time the > > slot reaches ReplicationSlotRelease, it is still in RS_EPHEMERAL state > > and is therefore dropped by release. OTIOH, a temporary slot is left > > behind. I think the temporary slot should also be dropped because the > > caller never received a reference to it. I don't see a legitimate use > > case where a temp slot should survive specifically because its > > creation call failed. > > While I agree that it would be an ideal behavior and the analysis > holds for logical slots, I want to note that persistent physical > replication slots are created with RS_PERSISTENT so if an error > happens during the slot creation the slot is left behind. Also, > logical persistent slots actually have the same gap: if > ReplicationSlotPersist() raises an error it leaves a persistent slot > behind as well. Given that slot creation and drop are not > transactional operations, and that leaving a slot behind on a failure > is not a new behavior, I'm inclined toward only releasing the slot at > the subxact abort. We can discuss the better behavior on HEAD > separately.
Yes, I realised the same. The ephemeral state only applies to logical slots, not to physical slots or temporary slots. I agree to keep the back-branch fix simple and solve the slot leak and crash reported in this thread. However, I think the creation failure on temporary slots inside a subxact also needs to be fixed in the back branches (perhaps separately), because one can hit the issue with direct SQL. A temporary slot whose creation fails needs to be dropped, to avoid leaking resources for a slot the caller never got a reference to. In the replication slot subxact callback, on the abort path, we need to know whether the slot's creation failed. Ephemeral slots already handle that, but only for persistent logical slots. A temporary slot stays RS_TEMPORARY throughout. So there are a few ways to solve this: 1/ Also mark temporary slots as ephemeral initially and transition them to RS_TEMPORARY once creation succeeds. A quick check shows this needs changes in many places. 2/ Introduce a new state to represent a temporary slot still in creation (RS_TEMPORARY_EPHEMERAL or such). 3/ Use a boolean in the ReplicationSlot structure (is_create_in_progress or such), and in the subxact callback, when the slot is temporary and is_create_in_progress is set, drop just that temporary slot and leave the others alone. I prefer option 3, to keep it simple without adding a new state, and because it is back-branch friendly. The new boolean lives only in memory and is not written to disk. To drop a single temporary slot, I'm thinking of moving the single-slot drop code out of ReplicationSlotCleanup() into an internal helper function. Thoughts? On Thu, Aug 27, 2026 at 5:30 AM Amit Kapila <[email protected]> wrote: > > True, but OTOH, won't we already clean up resources not directly > associated with subxact in AtEOSubXact_LargeObject() or > AtEOSubXact_Files()? I don't see any problem as far as the current > pattern of usage for slots. The new restriction this patch will add is > "a slot acquired in a subxact does not survive that subxact being > unwound." which should be okay because of its similarity with > top-level xact behavior. I feel if possible we should restrict such > usage explicitly in code in some way rather than one finding out this > as a surprise. Hi Amit, By restricting in the code, does that mean adding an Assert, or a WARNING, or a WARNING plus slot release (not an error), in the replication slot subxact callback on the commit path, instead of handing the slot off to the parent across the subtransaction boundary? -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
