Re: Persist slot invalidations before publishing them - Mailing list pgsql-hackers

On Fri, Sep 25, 2026 at 11:24 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> 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));
> "

Bertrand, I will come to this Assert soon. First I would like to
think/discuss if we can get rid of passing the 'update_inactive_since'
boolean altogether. Currently, we need it mainly for two reasons:

a) ReplicationSlotRelease() does not know when it should update
inactive_since and when it should skip it.
b) The slot-skip and other invalidation flows currently behave differently.

We could eliminate the second difference by making the logic same for
both the flows. I don't think there is any harm in skipping the
'inactive_since' update for the slot-sync's
slot-invalidation-persist's error case as well. We never use
'inactive_since' to invalidate idle synced slots (see
CanInvalidateIdleSlot()). And  'inactive_since' only matters for
synced slots after standby promotion, when it is reset for all synced
slots by update_synced_slots_inactive_since() from ShutDownSlotSync()
(promotion's flow). So I don't think we need to maintain separate
logic for this rare error case. If really needed in the future, we
could still preserve the current behavior IsSyncingReplicationSlots()
check in ReplicationSlotRelease(), but I don't think it is worth the
extra complexity.

That leaves us with just handling the failed-invalidation case where
ReplicationSlotRelease() need to avoid update of inactive_since. How
about using a static flag for this? We can set it in the CATCH block
of ReplicationSlotPersistInvalidation() before calling
ReplicationSlotRelease().

With this approach, both flows can use ReplicationSlotRelease() in the
same way and we don't need to split the logic into
ReplicationSlotReleaseInternal() either. I have attached a sample
patch. Please let me know your thoughts.

Attached patch applies atop v7-002.

thanks
Shveta