On Tue, Sep 29, 2026 at 1:59 PM Bertrand Drouvot <[email protected]> wrote: > > Hi, > > On Tue, Sep 29, 2026 at 11:07:34AM +0530, shveta malik wrote: > > > > > Bertrand, I will come to this Assert soon. First I would like to > > think/discuss if we can get rid of passing the 'update_inactive_since' > > boolean altogether. Currently, we need it mainly for two reasons: > > > > a) ReplicationSlotRelease() does not know when it should update > > inactive_since and when it should skip it. > > b) The slot-skip and other invalidation flows currently behave differently. > > > > We could eliminate the second difference by making the logic same for > > both the flows. I don't think there is any harm in skipping the > > 'inactive_since' update for the slot-sync's > > slot-invalidation-persist's error case as well. We never use > > 'inactive_since' to invalidate idle synced slots (see > > CanInvalidateIdleSlot()). And 'inactive_since' only matters for > > synced slots after standby promotion, when it is reset for all synced > > slots by update_synced_slots_inactive_since() from ShutDownSlotSync() > > (promotion's flow). So I don't think we need to maintain separate > > logic for this rare error case. If really needed in the future, we > > could still preserve the current behavior IsSyncingReplicationSlots() > > check in ReplicationSlotRelease(), but I don't think it is worth the > > extra complexity. > > Yeah, that makes sense. This is a rare error path, so I agree that it is not > worth the extra complexity. > > > That leaves us with just handling the failed-invalidation case where > > ReplicationSlotRelease() need to avoid update of inactive_since. How > > about using a static flag for this? We can set it in the CATCH block > > of ReplicationSlotPersistInvalidation() before calling > > ReplicationSlotRelease(). > > > > With this approach, both flows can use ReplicationSlotRelease() in the > > same way and we don't need to split the logic into > > ReplicationSlotReleaseInternal() either. I have attached a sample > > patch. Please let me know your thoughts. > > The static flag looks safe, but I wonder if it wouldn't be clearer to keep > ReplicationSlotReleaseInternal() and call it with false from the error path? > > That would still allow us to remove update_inactive_since from > ReplicationSlotPersistInvalidation() and its callers, while keeping the > exceptional release behavior explicit. >
Okay, works for me. thanks Shveta
