Re: Persist slot invalidations before publishing them

Hi,

Thanks for v2. I made the save fail with the injection point of 0001,
and the slot comes out of it fine: still valid, released by the
checkpointer, and I can drop it. On master the same failure leaves it
invalid but still owned by the checkpointer, and the next checkpoint
does not fix that:

active|active_pid|invalidation_reason|restart_lsn|wal_status
t|259023|wal_removed||lost

SELECT pg_drop_replication_slot('target_slot');
ERROR: replication slot "target_slot" is active for PID 259023

> If a second invalidator reaches the same slot in that window (say a
> restartpoint enforcing max_slot_wal_keep_size while startup is replaying
> a wal_level change, or the other way round), it seems it would take the
> "slot is in use" path, so startup might send a recovery conflict to
> active_pid, or another process a SIGTERM. What do you think?

Both happen, and the SIGTERM shuts the standby down. 0003 makes the
second invalidator wait for the first one instead, and adds a test with
the two processes in both orders.

The standby has max_slot_wal_keep_size = 1MB, a logical slot 8 segments
of 1MB behind, and the injection point of 0001 attached with 'wait' for
that slot. The primary is restarted with wal_level = replica, so that the
startup process invalidates the slot, and once it waits at the injection
point, CHECKPOINT on the standby:

checkpointer LOG: restartpoint starting: fast wait
checkpointer LOG: terminating process 378422 to release replication
slot "startup_first"
checkpointer DETAIL: The slot's restart_lsn 0/012002D0 exceeds the
limit by 7339312 bytes.
startup LOG: invalidating obsolete replication slot "startup_first"
postmaster LOG: startup process (PID 378422) exited with exit code 1
postmaster LOG: terminating any other active server processes
postmaster LOG: shutting down due to startup process failure

The other way round, the startup process finds the slot held by the
checkpointer and sends it a recovery conflict. The checkpointer errors
out at its next CHECK_FOR_INTERRUPTS(), and the startup process then
invalidates the slot itself:

startup LOG: terminating process 378420 to release replication slot
"checkpointer_first"
checkpointer ERROR: canceling statement due to conflict with recovery
client backend ERROR: checkpoint request failed
startup LOG: invalidating obsolete replication slot "checkpointer_first"

On master the slot is marked invalid under the spinlock at the moment it
is claimed, so the second invalidator sees the invalidation and does
nothing.

0003 takes the slot's io_in_progress_lock before looking at the slot,
and holds it from claiming the slot until the invalidation is published;
0001 holds it for the write only. Whoever comes second then sees either
an invalidated slot or one nobody is working on, and only signals a
process that really uses the slot. If the lock is not free, the function
drops ReplicationSlotControlLock, waits for it, and starts over, the same
way it does around the condition variable sleep. The test fails on v2 as
above and passes with 0003.

The side effects of 0003, as far as I can see:

(a) The second invalidator now waits (wait event ReplicationSlotIO)
instead of signalling; on master it never waited. The wait is one write
and fsync of the slot's state file, and only when two processes go for
the same slot at the same time.

(b) Every pass over the slots now takes and releases each in-use slot's
io_in_progress_lock once. A checkpoint's pass over N valid physical
slots, median of 60 checkpoints:

N v2 0003
10 1.5 us 1.6 us
100 7.2 us 9.0 us

A pass that finds a slot being written waits for that write, without
ReplicationSlotControlLock.

(c) The two locks cannot deadlock: no holder of an io_in_progress_lock
takes ReplicationSlotControlLock, since the callers of SaveSlotToPath()
hold ReplicationSlotAllocationLock at most, and the slot is not in use
yet when CreateSlotOnDisk() writes it. The second invalidator holds
nothing while it waits. Nothing changes for a slot that is really in use:
a walsender's slot, or a temporary slot of an idle session, is signalled
as before.

(d) Not covered: synchronize_one_slot() acquires the slot before it
takes the lock, so a restartpoint that looks at the slot in between still
terminates the slot sync worker. Master has the same window, between
acquiring the slot and publishing; closing it would take a flag in the
slot's shared memory.

I think 0003 is small enough to be backpatched with 0001.

Regards,
Rui