Hi,
On Fri, Sep 25, 2026 at 05:54:41AM +0000, Bertrand Drouvot wrote:
> Hi,
>
> On Fri, Sep 25, 2026 at 09:38:27AM +0530, shveta malik wrote:
> > On Thu, Sep 24, 2026 at 3:11 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > >
> >
> > Thanks for addressing comments. A few concerns on 001:
>
> Thanks for looking at it!
>
> > 1)
> >
> > In SaveSlotToPath(), should we add an 'Assert(cp.slotdata.restart_lsn
> > == InvalidXLogRecPtr)' at the end for the 'if (clear_restart_lsn)'
> > case?
> >
> > slot->data.invalidated = invalidation_cause;
> > if (clear_restart_lsn)
> > + {
> > + Assert(cp.slotdata.restart_lsn == InvalidXLogRecPtr);
> > slot->data.restart_lsn = InvalidXLogRecPtr;
> > + }
> >
> > While slot->last_saved_restart_lsn correctly inherits
> > cp.slotdata.restart_lsn on the next line, adding this Assert
> > guarantees that the removed logic from
> > InvalidatePossiblyObsoleteSlot() was successfully compensated for in
> > the on-disk struct before we propagate it to shared memory. It is not
> > mandatory, but it would be good to have.
>
> I’m not sure this assertion adds much, since cp.slotdata.restart_lsn is explicitly
> cleared above and is not modified afterward.
>
> > 2)
> > + Assert(update_inactive_since || slot->data.persistency == RS_PERSISTENT);
> >
> > In ReplicationSlotReleaseInternal(), I didn’t quite understand the
> > reasoning behind above Assert. Does this mean that when the caller
> > passes update_inactive_since=true, the slot can even be temporary,
> > whereas if we are not updating inactive_since, the slot must be
> > persistent?
>
> Yes. In fact, with update_inactive_since=true it can also be ephemeral, since
> ReplicationSlotRelease() uses that value for the ordinary release path.
>
> This is not specific to slotsync. The false case is introduced by 0001 and is
> only used to preserve inactive_since when rolling back ownership of an inactive
> persistent slot.
>
> Maybe the following comment would make that clearer?
>
> "
> /*
> * Skipping the inactive_since update is only needed when undoing the
> * internal acquisition of an inactive persistent slot after an ERROR.
> */
> "
>
> > 3)
> > Another doubt I have is that with above Assert, when
> > update_inactive_since is TRUE, we are even allowing RS_EPHEMERAL
> > slots. However, ReplicationSlotPersistInvalidation() explicitly
> > disallows them in patch002 with:
> >
> > Assert(slot->data.persistency != RS_EPHEMERAL);
> >
> > Both checks are not in sync.
>
> I think they apply to different scopes. ReplicationSlotReleaseInternal() is the
> general release implementation, so update_inactive_since=true imposes no
> persistency restriction. In particular, an ephemeral slot is dropped by that
> path.
>
> ReplicationSlotPersistInvalidation() has a narrower contract and is only
> intended for persistent or temporary slots. That said, maybe its Assert could
> express all the supported combinations more clearly?
>
> "
> Assert(slot->data.persistency == RS_PERSISTENT ||
> (slot->data.persistency == RS_TEMPORARY &&
> update_inactive_since));
> "
>
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]).
[1]: https://postgr.es/m/CAHWVJhHNt2vUs6xBPZ9c%2Bsh1HjLroC032%2BqKjN%2B_qh%2Bbvc_M4Q%40mail.gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
