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. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
