agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Zhijie Hou <houzhijie22@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>
Cc: JoongHyuk Shin <sjh910805@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Rui Zhao <zhaorui126@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
Date: Mon, 28 Sep 2026 07:46:06 +0000
Message-ID: <arobPgCnS2yTuxZb@bdtpg> (raw)
In-Reply-To: <CAFvd2n-UfYuUE8BiGpkD7CjAyULW5xW3d3kDD_+k=qS_9eP_Bw@mail.gmail.com>
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>
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
view thread (32+ messages) latest in thread
Message-ID: <arobPgCnS2yTuxZb@bdtpg>
Permalink: ../arobPgCnS2yTuxZb@bdtpg/
Also on: postgresql.org/message-id/arobPgCnS2yTuxZb@bdtpg
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bertranddrouvot.pg@gmail.com, houzhijie22@gmail.com, shveta.malik@gmail.com, sjh910805@gmail.com, amit.kapila16@gmail.com, zhaorui126@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Persist slot invalidations before publishing them
In-Reply-To: <arobPgCnS2yTuxZb@bdtpg>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox