Re: Persist slot invalidations before publishing them

Hi,

On Thu, Sep 24, 2026 at 5:41 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> On Thu, Sep 24, 2026 at 10:55:03AM +0530, shveta malik wrote:
> > 2)
> > + /* Let caller know */
> > + invalidated = true;
> > + LWLockRelease(&s->io_in_progress_lock);
> > ReplicationSlotRelease();
> >
> > Wouldn't it be better (and safer) to release the slot before releasing
> > the I/O lock?
> >
> > Currently, concurrent invalidators are protected by the
> > 'invalidation_cause == RS_INVAL_NONE' check after acquiring the lock.
> > But releasing the slot first would close this race window entirely. It
> > would also make the order consistent with Patch 002 and the
> > error-handling flow in Patch 001 itself.
>
> The current ordering should be safe because the invalidation has already been
> published, so a concurrent invalidator exits before considering active_proc.
>
> That said, I agree that releasing the slot first means this ordering no longer
> relies on that check and makes the success and error paths consistent. So, done
> in the attached.
>
> It also adds the check you suggested for the synchronized slot's shared memory
> state after a successful synchronization.

Thanks for updating the patches.

When reading the patches, the part that feels heavy to me is the serialization
machinery added to InvalidatePossiblyObsoleteSlot() for the two-invalidator
race - the conditional acquire of io_in_progress_lock, dropping
ReplicationSlotControlLock to wait, and the restart of the loop - plus the
caller-owns-the-io-lock contract that ReplicationSlotPersistInvalidation()
imposes on both call sites. I understand why it's needed once claim and publish
stop being atomic (the SIGTERM that kills the startup process and shuts down
the standby is nasty), but I wonder if we can avoid making them non-atomic in
the first place.

You mentioned effective_catalog_xmin, and there are similar shadow fields like
last_saved_restart_lsn. What about the same style here: keep the claim exactly
as on master - active_proc and data.invalidated set in one spinlock section -
and add a pure in-memory boolean, say invalidation_durable, set only at the
point the invalid image has actually been written and fsynced (the tail of
SaveSlotToPath(), keyed off the image just written. All consumer references to
data.invalidated (horizon computations, pg_replication_slots, slotsync's
skip/drop decisions) would consult the new flag instead; the invalidators'
mutual-exclusion check and the acquire path keep reading the cause as today.

The new flag can be added to the padding space, so there is no change in the
size of ReplicationSlot.

I'm not insisting that we change the approach, just wanted to share an
alternative for discussion.

Best Regards,
Zhijie Hou