Temporary slot leak when creation fails in a subtransaction

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+7V0vNXjLS31w@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 $$;

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com