Hi, Can you please point me to the PG19 patch on which I need to apply the patch shared on this thread ? I am interested in reviewing this work too.
Thank you, Narayanan On Tue, Sep 29, 2026 at 2:40 PM shveta malik <[email protected]> wrote: > On Tue, Sep 29, 2026 at 2:05 PM shveta malik <[email protected]> > wrote: > > > > On Fri, Sep 25, 2026 at 11:09 PM Bharath Rupireddy > > <[email protected]> wrote: > > > > > > Hi, > > > > > > Moving the discussion from [1] here, and CCing the people involved > there. > > > > > > A temporary slot (logical or physical) whose creation fails inside a > > > subtransaction, with the error caught by a PL/pgSQL exception block, > > > is left behind. Such a slot stays until the session ends, a top-level > > > error happens in that session, or the user drops it. A temporary > > > logical slot holds WAL and the catalog xmin, and the same session can > > > decode partial transactions from it (data loss for logical decoding). > > > A temporary physical slot holds WAL. > > > > > > Here is a reproducer for a temporary logical slot [2]. The slot is > > > left with restart_lsn and catalog_xmin set and confirmed_flush_lsn not > > > set. A slot in that state can still be used for decoding, and with no > > > confirmed_flush_lsn that can lose data (see the test case in the > > > attached patch). There is no start point, and since decoding from the > > > slot is not slot creation, the snapshot builder can restore a > > > serialized snapshot and become consistent in the middle of a > > > transaction that began before restart_lsn. Without the fix, the > > > healthy slot in the test returns the transaction with both inserts and > > > the leftover slot returns it with only the second one. > > > > > > A temporary physical slot can also be left behind this way, but only > > > when writing its state file fails after the slot is allocated, for > > > example when the disk runs out of space. > > > > > > The issue exists on HEAD and all supported branches back to PG14. The > > > fix in the other thread releases a slot that stays acquired after an > > > error is caught in a subtransaction. That release does not drop a > > > temporary slot whose creation failed. Even without that fix, on > > > non-assert builds the leftover slot just stays marked active and can > > > be decoded from, so the same data loss happens. > > > > > > A few ways to fix this were discussed in the other thread. > > > > > > 1/ Mark temporary slots as ephemeral initially and transition them to > > > temporary once creation succeeds. Invasive. Not back-patchable. > > > > > > 2/ Introduce a new state to represent a temporary slot still in > > > creation (RS_TEMPORARY_EPHEMERAL or such). Invasive. Makes the code > > > complex. Not back-patchable. > > > > > > 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 that boolean is set, drop just that slot and > > > leave the others alone. Not ABI compatible. Not back-patchable. > > > > > > 4/ At subxact abort, if the slot is a temporary logical slot and its > > > confirmed_flush is invalid, drop it. The drop needs to unset the > > > PROC_IN_LOGICAL_DECODING flag as ReplicationSlotRelease() does. > > > > > > 5/ Add a check to CreateDecodingContext() that raises an error if the > > > slot's confirmed_flush is invalid. On v17 and later, slot > > > synchronization needs to be excluded, since update_local_synced_slot() > > > advances a newly created synced slot via > > > LogicalSlotAdvanceAndCheckSnapState() before its confirmed_flush is > > > set. > > > > > > 6/ Track the in-progress creation with a backend-local variable > > > (something like MyReplicationSlotCreating), set when the slot is > > > created and reset when it is released. In the subxact callback, use it > > > to drop just the temporary slot being created. Back-patchable. Covers > > > both logical and physical temporary slots. > > > > > > I prefer the backend-local tracking approach and attached a patch for > > > it on top of v19 from the other thread. Please have a look. I also > > > think it is good to tighten this up with the CreateDecodingContext() > > > check, so that a slot with an invalid confirmed_flush can never be > > > used for decoding. > > > > > > Thoughts? > > > > > > [1] > https://www.postgresql.org/message-id/CAHg+QDeuf9tCq3ce=kgFMJP0m=PZC+wi6B=ys+7v0vnxjls...@mail.gmail.com > > > > > > [2] > > > DO $$ > > > BEGIN > > > PERFORM pg_create_logical_replication_slot('tmp_bad', > 'no_such_plugin', true); > > > EXCEPTION WHEN OTHERS THEN > > > RAISE NOTICE 'caught %', SQLSTATE; > > > END $$; > > > > > > > Thanks for the patch Bharath. The patch does not apply to HEAD (even > > on an older version dated 15 days ago). Can you please rebase it? > > > > Kuroda-san informed me off-list that it applies atop PG19 patch shared > in another thread. I will review. > But one question: are we planning to push this fix too to PG19 and > thus the patch is this way? And then later we can do it on HEAD, is > that the plan? > > thanks > Shveta > > >
