Hi, On Thu, Sep 24, 2026 at 5:41 PM Bertrand Drouvot <[email protected]> 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
