Re: Persist slot invalidations before publishing them

Hi,

On Sun, Oct 4, 2026 at 11:30 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Wed, Sep 30, 2026 at 03:50:30PM +0530, Ashutosh Sharma wrote:
> > Hi,
> >
> > On Mon, Sep 28, 2026 at 9:16 PM Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > >
> > > The patch needed a rebase, so at the same time I went ahead with the proposed
> > > changes above (plus the one in the commit message suggested by Rui in [1]).
> > >
> >
> > Thanks for reporting the problem and providing a patch for it. The
> > approach looks good to me, but I have a few comments to share:
>
> Thanks for looking at it!
>
> >
> > I think it would be good to add a comment above
> > InvalidatePossiblyObsoleteSlot() explaining the reason for this change
> > in the usual slot update pattern.
>
> What about something like?
>
> "
> Unlike normal replication slot updates, persist the invalidation before
> publishing it in shared memory. Publishing it first could allow resource
> horizon computations to remove resources required by the slot before the
> invalidation reaches disk. If saving then failed, a restart could restore
> the old valid slot. Keeping the shared slot valid until the invalidated
> image is durable avoids that state.
> "

Looks clear to me.

>
> >
> > 2)
> >
> > + {
> > + SaveSlotToPath(slot, path, ERROR, cause, clear_restart_lsn);
> > + }
> >
> > Do we need to pass clear_restart_lsn here? Cause can be used to
> > determine the restart lsn value later, no?
>
> I don't think so. InvalidatePossiblyObsoleteSlot() clears restart_lsn for
> RS_INVAL_WAL_REMOVED, while slotsync must preserve the local restart_lsn
> when copying the same invalidation cause. So the cause alone is not enough to
> determine the desired behavior.
>

Okay got it, then it does make sense to have this clear restart lsn flag.

> > 3)
> >
> > The current patch changes the interface for SaveSlotToPath() (), isn't
> > it possible to keep it unchanged, that would probably also reduce some
> > amount of complexity.
>
> Yeah, that seems worthwhile. I'll change it that way.
>

Thanks. I'll review again once the updated patch is posted.

--
With Regards,
Ashutosh Sharma.