Date: Sun, 4 Oct 2026 05:59:59 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Ashutosh Sharma <ashu.coek88@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: <asHrX+DPqiuK8IR6@bdtpg>
References: <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>
 <arqLwLn0w9DdfSZG@bdtpg>
 <CAE9k0Pn1THJ0Hw6bZKSrZUnvxSrh19=PAu85Ka=99LzGki750g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAE9k0Pn1THJ0Hw6bZKSrZUnvxSrh19=PAu85Ka=99LzGki750g@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/asHrX%2BDPqiuK8IR6%40bdtpg>
Precedence: bulk

Hi,

On Wed, Sep 30, 2026 at 03:50:30PM +0530, Ashutosh Sharma wrote:
> Hi,
> 
> On Mon, Sep 28, 2026 at 9:16 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > The patch needed a rebase, so at the same time I went ahead with the proposed
> > changes above (plus the one in the commit message suggested by Rui in [1]).
> >
> 
> Thanks for reporting the problem and providing a patch for it. The
> approach looks good to me, but I have a few comments to share:

Thanks for looking at it!

> 
> I think it would be good to add a comment above
> InvalidatePossiblyObsoleteSlot() explaining the reason for this change
> in the usual slot update pattern.

What about something like?

"
Unlike normal replication slot updates, persist the invalidation before
publishing it in shared memory. Publishing it first could allow resource
horizon computations to remove resources required by the slot before the
invalidation reaches disk. If saving then failed, a restart could restore
the old valid slot. Keeping the shared slot valid until the invalidated
image is durable avoids that state.
"

> 
> 2)
> 
> + {
> + SaveSlotToPath(slot, path, ERROR, cause, clear_restart_lsn);
> + }
> 
> Do we need to pass clear_restart_lsn here? Cause can be used to
> determine the restart lsn value later, no?

I don't think so. InvalidatePossiblyObsoleteSlot() clears restart_lsn for
RS_INVAL_WAL_REMOVED, while slotsync must preserve the local restart_lsn
when copying the same invalidation cause. So the cause alone is not enough to
determine the desired behavior.

> 3)
> 
> The current patch changes the interface for SaveSlotToPath() (), isn't
> it possible to keep it unchanged, that would probably also reduce some
> amount of complexity.

Yeah, that seems worthwhile. I'll change it that way.

Regards,

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