On Fri, Feb 7, 2025 at 4:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Feb 7, 2025 at 8:00 AM Peter Smith <smithpb2250@gmail.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
