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

On Fri, Feb 7, 2025 at 4:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Feb 7, 2025 at 8:00 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > ======
> > src/backend/access/transam/xlog.c
> >
> > 1.
> > XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
> > KeepLogSeg(recptr, &_logSegNo);
> > - if (InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_REMOVED,
> > + if (InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_REMOVED |
> > RS_INVAL_IDLE_TIMEOUT,
> > _logSegNo, InvalidOid,
> > InvalidTransactionId))
> > {
> > @@ -7792,7 +7792,7 @@ CreateRestartPoint(int flags)
> > replayPtr = GetXLogReplayRecPtr(&replayTLI);
> > endptr = (receivePtr < replayPtr) ? replayPtr : receivePtr;
> > KeepLogSeg(endptr, &_logSegNo);
> > - if (InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_REMOVED,
> > + if (InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_REMOVED |
> > RS_INVAL_IDLE_TIMEOUT,
> > _logSegNo, InvalidOid,
> > InvalidTransactionId))
> >
> > It seems fundamentally strange to me to assign multiple simultaneous
> > causes like this. IMO you can't invalidate something that is invalid
> > already. I gues v71 was an attempt to implement Amit's:
> >
>
> The idea is to invalidate the slot either due to WAL_REMOVED or
> IDLE_TIMEOUT in one go during the checkpoint instead of taking
> multiple passes over the slots during the checkpoint. Feel free to
> suggest if you can think of a better way to implement it.
>

Hi Amit,

My preference already suggested was to have a separation between the
concepts of *actual* causes (e.g. discrete enum values like in v70)
and *possible* causes to be checked (using #defines for bit flags).

My v72-0001 review [1] includes a top-up patch to show what doing it
this way might look like.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPupn_S0mrM2zB%2BFwAbPqVak7jwSjRhU3WyA18QC1HU__g%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia