Re: Introduce XID age and inactive timeout based replication slot invalidation

On Tue, Jan 21, 2025 at 8:26 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some review comments for patch v61-0002
>
> ======
> src/backend/replication/slot.c
>
> 1.
> * Whether a slot needs to be invalidated depends on the cause. A slot is
> - * removed if it:
> + * invalidated if it:
> * - RS_INVAL_WAL_REMOVED: requires a LSN older than the given segment
> * - RS_INVAL_HORIZON: requires a snapshot <= the given horizon in the given
> * db; dboid may be InvalidOid for shared relations
> * - RS_INVAL_WAL_LEVEL: is logical
> + * - RS_INVAL_IDLE_TIMEOUT: idle slot timeout has occurred
>
> 1a.
> Firstly the wording seems odd. "Is invalidated if it:" (missing words?)
>
> ~
>
> 1b.
> Secondly, is this comment strictly correct? IIUC it's not *always*
> going to be invalidated just because the cause is one of those listed.
> e.g. the code calls InvalidatePossiblyObsoleteSlot but it might not
> end up invalidating the slot having a cause RS_INVAL_IDLE_TIMEOUT.
>

I feel the phrase "A slot is invalidated if it:" is supposed to be
read alongside the respective cause description, such as:
"A slot is invalidated if it requires an LSN older than…"
"A slot is invalidated if it requires a snapshot <= the…"
"A slot is invalidated if it is logical"

IIUC, each listed cause specifies a clear condition under which the
slot should *always* be invalidated for that cause. To maintain
consistency with the header line "A slot is invalidated if it:", I’ve
modified the description/condition for RS_INVAL_IDLE_TIMEOUT
accordingly. Also, corrected the RS_INVAL_WAL_LEVEL description.

Attached the v62 patch set with above mentioned changes and also
addressed comments at [1].

[1] https://www.postgresql.org/message-id/CAHut%2BPvC4uPabeGMvDuTQ4S%2B5eX66Y6%2BtU5QMRmB2jDw-Cj2Cw%40mail.gmail.com

--
Thanks,
Nisha