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

On Wed, Nov 27, 2024 at 8:39 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Nisha,
>
> Here are some review comments for the patch v50-0002.
>
> ======
> src/backend/replication/slot.c
>
> InvalidatePossiblyObsoleteSlot:
>
> 1.
> + if (now &&
> + TimestampDifferenceExceeds(s->inactive_since, now,
> + replication_slot_inactive_timeout_sec * 1000))
>
> Previously this was using an additional call to SlotInactiveTimeoutCheckAllowed:
>
> + if (SlotInactiveTimeoutCheckAllowed(s) &&
> + TimestampDifferenceExceeds(s->inactive_since, now,
> + replication_slot_inactive_timeout * 1000))
>
> Is it OK to skip that call? e.g. can the slot fields possibly change
> between assigning the 'now' and acquiring the mutex? If not, then the
> current code is fine. The only reason for asking is because it is
> slightly suspicious that it was not done this "easy" way in the first
> place.
>
Good catch! While the mutex was being acquired right after the now
assignment, there was a rare chance of another process modifying the
slot in the meantime. So, I reverted the change in v51. To optimize
the SlotInactiveTimeoutCheckAllowed() call, it's sufficient to check
it here instead of during the 'now' assignment.

Attached v51 patch-set addressing all comments in [1] and [2].

[1] https://www.postgresql.org/message-id/CAHut%2BPtuiQj1hwm%3D73xJ8hWuw-9cXbN4dHJHpM6EXxubDJgmFA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPvi-g%2B9%2Bhjmjg44OzTN9L3YGQiCXBDAVaTVWvSn5SSwmw%40mail.gmail.com

--
Thanks,
Nisha