Re: Persist slot invalidations before publishing them

On Tue, Sep 29, 2026 at 1:59 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> 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