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
> <[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.
>
> 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
