Re: [BUG] [PATCH] Allow physical replication slots to recover from archive after invalidation

Hi Sravan,

Thanks a lot for the detailed review. Attached is v5, which addresses
your comments. Details below.

> The fix produces an additional discrepancy in the slot status,
> which is very unlikely to occur, through a phantom false clear.
> A buffered 'r' reply from downstream can revalidate a slot for
> a short duration before its invalidation again when the WAL is
> not found. [...] It may be worth looking at how we can avoid
> it, possibly by clearing the flag on a successful
> restart_lsn advancement past the invalidation point, or after a
> successful WAL send.

Fixed in v5, following the second of your two suggestions in a
slightly stronger form: a successful WAL send plus a confirmation
that the standby received it.
The walsender now keeps a per-session target LSN (revalidationTargetPtr),
invalid until XLogSendPhysical() establishes it, once per session:

- If there is WAL to send, the target is the end LSN of the first
WAL data message queued. Queueing it proves WALRead() succeeded,
so for wal_removed the required WAL demonstrably exists.

- If the standby is already caught up (SendRqstPtr <= sentPtr), the
target is the current send position. Since the qualifying reply may
have already been processed (the initial status reply) and a
caught-up standby may never send another one, the confirmed flush
position is rechecked immediately at that point.

PhysicalConfirmReceivedLocation() clears the invalidation only when a
flush ACK covers the target, atomically with the restart_lsn update
under the slot spinlock, and persists the slot right away. A buffered
'r' processed before the target exists can no longer revalidate the
slot, so the phantom false clear and its misleading log line are gone.

I went with send-plus-ACK rather than restart_lsn advancement past the
invalidation point because the latter defers revalidation indefinitely
in a common case: after an idle_timeout invalidation the standby is
typically fully caught up, so on reconnect restart_lsn has nothing to
advance past until new WAL is generated, and the slot would sit
active-but-invalid on an idle primary even though the standby verifiably
holds everything. The ACK-covers-target gate provides the same
protection against the stray-'r' problem without that deferral. The
caught-up case cannot produce a phantom either: the confirmed flush
position covers all WAL the server has, so there is nothing left to
read that could subsequently fail and re-invalidate the slot.

> For a slot that is revalidatable but not recoverable, during the
> window between slot acquire and the first flush acknowledgement,
> the slot will show up as active while invalid for a slightly
> longer period, and continues to happen on every retry. To report
> correctly during such windows, we can check for WAL availability
> before slot acquire (before going active).

I looked at this but left it out of v5. A pre-acquire availability
check is inherently racy, a checkpoint can remove the WAL between
the check and the first read, as you noted yourself for the
recoverable case, so it can shorten the window but not close it,
at the cost of an extra probe on a path that is retried anyway. I am
not opposed to it as an incremental improvement if the window proves
bothersome in practice.

> Can we consider allowing auto_revalidate via a GUC - to provide
> the option across a fleet of slots, for convenience?

I would prefer to keep it per-slot for now: a GUC would change the
invalidation contract of slots whose owners may not expect it, and
the interaction between a reloadable global setting and the per-slot
flag needs its own discussion, it would also increase the scope of this patch.
It could be added later on top of this series if there is demand.
Happy to hear other opinions.

> So: is the intended contract of ReplicationSlotAcquire() to be the
> central invalidation gatekeeper [...] or an acquisition mechanism,
> in which case the invalidation policy (error/warn/attempt) could
> stay with each caller and only the error message be single-sourced
> via a small shared helper? [...] I lean toward the shared-helper
> approach, but I think the contract is the authors' call.

Agreed, and v5 takes the shared-helper approach you lean toward.
slot.c now exports ReplicationSlotInvalidationError(), which both
ReplicationSlotAcquire() (for error_if_invalid) and StartReplication()
(for the non-revalidatable branch) use, preserving the message consistency
goal of f41d8468. ReplicationSlotAcquire() stays a plain acquisition
mechanism and the revalidation policy lives entirely in the walsender.
Thanks also for confirming the retry behavior for unrecoverable slots,
and for checking the pg_receivewal, pg_basebackup, and upgrade angles.
I agree there is nothing to do there. One side effect of the new gate
worth mentioning: a caught-up standby now revalidates promptly on
reconnect instead of waiting for new WAL, so the documentation in
0005 was updated accordingly.

The docs in 0001/0005 and the commit messages were updated to describe
the new gating. All recovery TAP tests pass, including the new
056_auto_revalidate_physical_slot.pl.

--
Best regards,
João Foltran