On Sat, Aug 29, 2026 at 4:42 AM Bharath Rupireddy <[email protected]> wrote: > > 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.
Do you mean one "cannot" 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. My preference will be option 1 but it needs careful checking for existing usages. > 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
