Re: Question about InvalidatePossiblyObsoleteSlot()

Hi,

On Tue, Oct 14, 2025 at 04:06:44PM -0700, Masahiko Sawada wrote:
> On Sun, Oct 12, 2025 at 11:27 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > Hi,
> >
> > On Thu, Oct 09, 2025 at 10:49:39AM +0800, suyu.cmj wrote:
> > > Hi,
> > > Thank you for the reference to commit 818fefd8fd4 and the related discussion thread. I understand the intent of
introducinginitial_restart_lsn was to preserve a consistent invalidation cause throughout the invalidation loop.
 
> > > However, I still have a few concerns about this design change:
> > > 1. I understand the intention to keep the invalidation cause consistent, but If a slot's restart_lsn advances
significantlyduring the invalidation check—indicating it is actively in use—shouldn't we reconsider invalidating it?
 
> > > 2. What potential issues arise if we refrain from invalidating slots whose restart_lsn advances during the
invalidationprocess? Intuitively, an actively used slot that has moved it's restart_lsn beyond the problematic point
shouldnot be marked invalid.
 
> > > 3. If the current approach is indeed correct, should we consider making PG15 and earlier consistent with this
behavior?The behavioral difference across versions may lead to different operational outcomes in otherwise similar
situations.
> > > I would appreciate your insights on these points.
> >
> > I agree that before 818fefd8fd4 the invalidation cause could move from
> > RS_INVAL_WAL_REMOVED to RS_INVAL_NONE if the slot restart lsn has been able to
> > advance enough between the time we release the mutex and do the next check.
> >
> > With 818fefd8fd4 that's not the case anymore and we keep WAL_REMOVED as the
> > invalidation cause (even if  the slot restart lsn has been able to advance
> > enough).
> >
> > That looks safe to use the pre 818fefd8fd4 behavior for the slot restart lsn
> > case because the WAL files have not yet been removed by the checkpointer/startup
> > process when it's busy in InvalidatePossiblyObsoleteSlot().
> >
> > I think that we could get rid of the initial_restart_lsn and just use
> > s->data.restart_lsn here (while keeping initial xmin ones to preserve the
> > intent of 818fefd8fd4 for those).
> 
> IIUC

Thanks for looking at it!

> with the proposed patch, it's possible that we report the slot
> invalidation once but don't actually invalidate the slot if slot's
> restart_lsn gets advanced and becomes greater than the oldestLSN after
> the report, is that right?

We don't really report an "invalidation", what we report is:

LOG:  terminating process 3998707 to release replication slot "logical_slot"
DETAIL:  The slot's restart_lsn 0/00842480 exceeds the limit by 2874240 bytes.
HINT:  You might need to increase "max_slot_wal_keep_size".

and we terminate the process:

FATAL:  terminating connection due to administrator command

We are not reporting:

DETAIL:  This replication slot has been invalidated due to "wal_removed".

and the slot is still valid.

That's the pre 818fefd8fd4 behavior.

Ideally, I think that we should not report anything and not terminate the
process. I did not look at it, maybe we could look at it as a second step (first
step being to restore the pre 818fefd8fd4 behavior)?

Regards,

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