Date: Tue, 6 Oct 2026 07:33:11 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: Ashutosh Sharma <ashu.coek88@gmail.com>,
	JoongHyuk Shin <sjh910805@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Rui Zhao <zhaorui126@gmail.com>, pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
Message-ID: <asSkN5GOEOKQZs9w@bdtpg>
References: 
 <CAJpy0uBDGfq8sermY3CE9M_LeBx+fAWsiKQLSZkpGeUOzEgcwg@mail.gmail.com>
 <arTwLinbxvVkEqu2@bdtpg>
 <CAJpy0uDASVicCkn-BJtecgpS081UXf4kG7UPicCB=cMzVJveMA@mail.gmail.com>
 <arYModKZ2uLwUyES@bdtpg>
 <arqLwLn0w9DdfSZG@bdtpg>
 <CAE9k0Pn1THJ0Hw6bZKSrZUnvxSrh19=PAu85Ka=99LzGki750g@mail.gmail.com>
 <asHrX+DPqiuK8IR6@bdtpg>
 <CAE9k0P=aea5vARHSEuDh5MRkUheqFHc_MV7CFgv96Ai8-Xuu3A@mail.gmail.com>
 <asOlG7/R4X5v7KlL@bdtpg>
 <CAJpy0uDw2b+Ykx73E6Xk6Cgo1qL_HOkoOTEv0OU5VsHXkJm2WQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAJpy0uDw2b+Ykx73E6Xk6Cgo1qL_HOkoOTEv0OU5VsHXkJm2WQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/asSkN5GOEOKQZs9w%40bdtpg>
Precedence: bulk

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.pg@gmail.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.

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?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com