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?
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
