On Wed, Nov 27, 2024 at 8:39 AM Peter Smith <smithpb2...@gmail.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
v51-0001-Add-error-handling-while-acquiring-a-replication.patch
Description: Binary data
v51-0002-Introduce-inactive_timeout-based-replication-slo.patch
Description: Binary data
