Re: Persist slot invalidations before publishing them

On Tue, Oct 6, 2026 at 1:03 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Tue, Oct 06, 2026 at 10:25:35AM +0530, shveta malik wrote:
> > On Mon, Oct 5, 2026 at 6:54 PM Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Oct 05, 2026 at 03:22:53PM +0530, Ashutosh Sharma wrote:
> > > > Thanks. I'll review again once the updated patch is posted.
> > >
> > > Thanks! Here it is.
> > >
> >
> > Since we have now modularized this further by introducing
> > SaveSlotToPathInternal() and SaveInvalidatedSlotToPath(), can we make
> > SaveSlotToPathInternal() consistent across both flows with respect to
> > lock acquisition and release?
> >
> > We could have SaveSlotToPath() acquire and release the lock,
> > preferably within a PG_TRY/PG_CATCH block. Additionally, the
> > 'was_dirty' check can also be moved up into SaveSlotToPath(), since
> > SaveInvalidatedSlotToPath() always forces a write and doesn't need the
> > check.
> >
> > This way, SaveSlotToPath() would acquire the lock only when the slot
> > is dirty, and SaveSlotToPathInternal() would not need to handle lock
> > acquisition/release based on the 'cause' argument. This would also let
> > us remove the multiple if blocks that currently check the 'cause' and
> > release the lock. Thoughts?
>
> Thanks for the proposal!
>
> I looked at it, but I think that it makes ordinary save errors get reported while
> holding io_in_progress_lock. In particular, the LOG path would perform logging
> and check for interrupts before releasing the lock.

I considered this when posting the comment, but then thought that the
other flow, 'SaveInvalidatedSlotToPath' is no different and has the
same limitations. Thus, I thought it should be okay if both flows
share the same logic (and limitations too).
But on re-thinking, I think you are worried about the flow from
CheckPointReplicationSlots() which only logs issues instead of
erroring out. I see your point now.

>
> As this is meant to be backpatched, I'm not sure it's worth changing the existing
> behavior as part of this fix. The proposed cleanup could be considered separately
> on HEAD as a follow up patch though.
>
> What do you think?
>

Okay, works for me.

thanks
Shveta