From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Ashutosh Sharma <ashu.coek88@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>
Cc: JoongHyuk Shin <sjh910805@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Rui Zhao <zhaorui126@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
Date: Tue, 6 Oct 2026 13:46:24 +0000
Message-ID: <asT7sGOzzQL/8hJl@bdtpg> (raw)
In-Reply-To: <CAE9k0P=AHtGdDcu+0-_zG=qkU337BeVqbtyPyATeoza2UudV0A@mail.gmail.com>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bertranddrouvot.pg@gmail.com, ashu.coek88@gmail.com, shveta.malik@gmail.com, sjh910805@gmail.com, amit.kapila16@gmail.com, zhaorui126@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
In-Reply-To: <asT7sGOzzQL/8hJl@bdtpg>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox