Date: Tue, 6 Oct 2026 13:46:24 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Ashutosh Sharma <ashu.coek88@gmail.com>
Cc: shveta malik <shveta.malik@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: <asT7sGOzzQL/8hJl@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>
 <CAE9k0P=AHtGdDcu+0-_zG=qkU337BeVqbtyPyATeoza2UudV0A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAE9k0P=AHtGdDcu+0-_zG=qkU337BeVqbtyPyATeoza2UudV0A@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/asT7sGOzzQL/8hJl%40bdtpg>
Precedence: bulk

Hi,

On Tue, Oct 06, 2026 at 03:46:24PM +0530, Ashutosh Sharma wrote:
> Hi,
> 
> 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.
> 
> Thanks, the attached patch looks good overall.

Thanks for looking at it!

> I only have a couple of
> concerns as of now, feel free to disregard them if you do not think
> they are worth addressing.
> 
> 1) Although assertions are useful, some appear redundant across the
> three layers of the slot invalidation path:
> 
> - cause != RS_INVAL_NONE is asserted in both
> ReplicationSlotPersistInvalidation() and SaveInvalidatedSlotToPath().
> - I/O lock ownership is asserted in those two functions and
> conditionally in SaveSlotToPathInternal().
> - The clear_restart_lsn constraint is asserted in both the public and
> internal functions.
> - cause == RS_INVAL_NONE || elevel >= ERROR is already guaranteed by
> the wrapper functions, since SaveInvalidatedSlotToPath() always passes
> ERROR.
> 
> I think some of these assertions could be removed, particularly the
> following ones:
> 
> static void
> SaveInvalidatedSlotToPath(ReplicationSlot *slot, const char *dir,
>                           ReplicationSlotInvalidationCause cause,
>                           bool clear_restart_lsn)
> {
>     Assert(cause != RS_INVAL_NONE);
>     Assert(LWLockHeldByMeInMode(&slot->io_in_progress_lock, LW_EXCLUSIVE));

I'm inclined to keep them. They check the preconditions at different layers.
There are similar caller/callee examples in the tree, for example:

SnapBuildSnapDecRefcount() / SnapBuildFreeSnapshot()
MemoryContextReset() / MemoryContextResetOnly()
MemoryContextDelete() / MemoryContextDeleteOnly()
SyncRepWakeQueue() / SyncRepQueueIsOrderedByLSN()

In particular, the cause assertion prevents RS_INVAL_NONE from reaching the regular
save path and trying to acquire the lock already held by the caller.

> 
> 2) SaveSlotToPathInternal() currently has several conditional branches
> distinguishing between the normal-save and invalidation-save paths. It
> might be worth considering whether this distinction can be simplified,
> making the function easier to follow.

I replied to a similar suggestion from Shveta in [1].

> Other than these points, the current patch set looks good to me. I
> will spend some more time reviewing it and report back if I find
> anything else worth mentioning.

Thanks!

[1]: https://www.postgresql.org/message-id/asSkN5GOEOKQZs9w%40bdtpg

Regards,

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