On Tue, Oct 6, 2026 at 1:03 PM Bertrand Drouvot <[email protected]> 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 > > <[email protected]> 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
