Re: Persist slot invalidations before publishing them

Hi,

On Mon, Oct 5, 2026 at 6:54 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)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. 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));

--

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.

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.

--
With Regards,
Ashutosh Sharma.