Date: Tue, 29 Sep 2026 08:29:49 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: 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: <art2/XiyZ0ZLG/da@bdtpg>
References: 
 <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>
 <CAJpy0uDASVicCkn-BJtecgpS081UXf4kG7UPicCB=cMzVJveMA@mail.gmail.com>
 <arYModKZ2uLwUyES@bdtpg>
 <CAJpy0uADOggVZJ3Z5E8C1K8D21i6PLgGB+=W+SZ9-5rn+GbYVg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAJpy0uADOggVZJ3Z5E8C1K8D21i6PLgGB+=W+SZ9-5rn+GbYVg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/art2/XiyZ0ZLG/da%40bdtpg>
Precedence: bulk

Hi,

On Tue, Sep 29, 2026 at 11:07:34AM +0530, shveta malik wrote:
> >
> Bertrand, I will come to this Assert soon. First I would like to
> think/discuss if we can get rid of passing the 'update_inactive_since'
> boolean altogether. Currently, we need it mainly for two reasons:
> 
> a) ReplicationSlotRelease() does not know when it should update
> inactive_since and when it should skip it.
> b) The slot-skip and other invalidation flows currently behave differently.
> 
> We could eliminate the second difference by making the logic same for
> both the flows. I don't think there is any harm in skipping the
> 'inactive_since' update for the slot-sync's
> slot-invalidation-persist's error case as well. We never use
> 'inactive_since' to invalidate idle synced slots (see
> CanInvalidateIdleSlot()). And  'inactive_since' only matters for
> synced slots after standby promotion, when it is reset for all synced
> slots by update_synced_slots_inactive_since() from ShutDownSlotSync()
> (promotion's flow). So I don't think we need to maintain separate
> logic for this rare error case. If really needed in the future, we
> could still preserve the current behavior IsSyncingReplicationSlots()
> check in ReplicationSlotRelease(), but I don't think it is worth the
> extra complexity.

Yeah, that makes sense. This is a rare error path, so I agree that it is not
worth the extra complexity.

> That leaves us with just handling the failed-invalidation case where
> ReplicationSlotRelease() need to avoid update of inactive_since. How
> about using a static flag for this? We can set it in the CATCH block
> of ReplicationSlotPersistInvalidation() before calling
> ReplicationSlotRelease().
> 
> With this approach, both flows can use ReplicationSlotRelease() in the
> same way and we don't need to split the logic into
> ReplicationSlotReleaseInternal() either. I have attached a sample
> patch. Please let me know your thoughts.

The static flag looks safe, but I wonder if it wouldn't be clearer to keep
ReplicationSlotReleaseInternal() and call it with false from the error path?

That would still allow us to remove update_inactive_since from
ReplicationSlotPersistInvalidation() and its callers, while keeping the
exceptional release behavior explicit.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com