Date: Mon, 28 Sep 2026 07:46:06 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Zhijie Hou <houzhijie22@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>,
	JoongHyuk Shin <sjh910805@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Rui Zhao <zhaorui126@gmail.com>, pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
Message-ID: <arobPgCnS2yTuxZb@bdtpg>
References: 
 <CAJpy0uDAyMbD61iJE-2qRMCtdQ4Qtuq+hy=mqDszFapgCHXaJQ@mail.gmail.com>
 <arNsgodAHkkxbxvX@bdtpg>
 <CAJpy0uBQgPf3R04RZp5BxfgABeDOyZdwm44hSOWA4JJraSo-_A@mail.gmail.com>
 <arOP6DEhgKcsNS5b@bdtpg>
 <CAJpy0uAiqT-__cV-MWNCQTXpAjcVDNh53+L2-DzZnYH8O5K1Vg@mail.gmail.com>
 <arP4wN5HW8h0Sr90@bdtpg>
 <CAJpy0uCeW9Kf1Ofgn=ojiG-FEpsf=KVgpF-289A_j9xX0G5AyA@mail.gmail.com>
 <CAJpy0uBDGfq8sermY3CE9M_LeBx+fAWsiKQLSZkpGeUOzEgcwg@mail.gmail.com>
 <arTwLinbxvVkEqu2@bdtpg>
 <CAFvd2n-UfYuUE8BiGpkD7CjAyULW5xW3d3kDD_+k=qS_9eP_Bw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAFvd2n-UfYuUE8BiGpkD7CjAyULW5xW3d3kDD_+k=qS_9eP_Bw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/arobPgCnS2yTuxZb%40bdtpg>
Precedence: bulk

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