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

On Mon, Sep 28, 2026 at 1:16 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Sun, Sep 27, 2026 at 10:10:33PM +0800, Zhijie Hou wrote:
> > When reading the patches,
>
> Thanks for looking at it!
>
> > 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.
>
> That might look heavy but I don't think this pattern is unusual: SLRU uses the
> same general lock, wait, and recheck pattern, and InvalidatePossiblyObsoleteSlot()
> already follows that model when waiting on active_cv.
>
> > 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.
>
> I'm not sure the alternative is lighter overall. It moves the complexity into
> a new intermediate slot state and requires each consumer of data.invalidated to
> decide whether it should also check invalidation_durable.
>
> > The new flag can be added to the padding space, so there is no change in the
> > size of ReplicationSlot.
>
> Yeah that look ok if, for example, we place it here:
>
> (gdb) ptype /o struct ReplicationSlot
> /* offset      |    size */  type = struct ReplicationSlot {
> /*      0      |       1 */    slock_t mutex;
> /*      1      |       1 */    _Bool in_use;
> /* XXX  2-byte hole      */
> /*      4      |       4 */    ProcNumber active_proc;
> /*      8      |       1 */    _Bool just_dirtied;
> /*      9      |       1 */    _Bool dirty;
> /* XXX  2-byte hole      */
> /*     12      |       4 */    TransactionId effective_xmin;
> .
> .
> .
>
> My concern is that data.invalidated would then have two roles depending on
> invalidation_durable. Invalidators and the acquisition path would treat a value
> other than RS_INVAL_NONE as an invalidation, while other consumers would do so
> only once invalidation_durable is set.
>
> That could be an issue for existing extensions on back branches. An extension
> could treat the slot as invalid while core consumers gated by invalidation_durable
> still treat the invalidation as not effective. So, although the ABI layout would
> be preserved, the semantics of an existing field would change.
>
> Thoughts?

I think the new approach could make the code significantly more
fragile and could lead to silent system corruption if not handled
carefully. For example, we could hit a case where logical decoding is
disabled on seeing the last invalidated slot and WALs are removed, but
then the fsync to disk fails. This would make the system completely
unrecoverable (system startup would fail because logical decoding was
disabled while a valid slot is still present on disk). Although we
will theoretically change all readers of invalidated to also consult
the shadow flag, my point is that a single missed check would result
in an unrecoverable state.

And not just existing extensions but even future ones could easily
fall into this exact trap by missing just one check.

thanks
Shveta