Hi,
On Sun, Oct 4, 2026 at 11:30 AM Bertrand Drouvot
<[email protected]> 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
> > <[email protected]> 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.
