Re: Introduce XID age and inactive timeout based replication slot invalidation

Hi,

On Mon, Apr 01, 2024 at 08:47:59AM +0530, Bharath Rupireddy wrote:
> On Fri, Mar 29, 2024 at 9:39 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Commit message states: "why we can't just update inactive_since for
> > synced slots on the standby with the value received from remote slot
> > on the primary. This is consistent with any other slot parameter i.e.
> > all of them are synced from the primary."
> >
> > The inactive_since is not consistent with other slot parameters which
> > we copy. We don't perform anything related to those other parameters
> > like say two_phase phase which can change that property. However, we
> > do acquire the slot, advance the slot (as per recent discussion [1]),
> > and release it. Since these operations can impact inactive_since, it
> > seems to me that inactive_since is not the same as other parameters.
> > It can have a different value than the primary. Why would anyone want
> > to know the value of inactive_since from primary after the standby is
> > promoted?
> 
> After thinking about it for a while now, it feels to me that the
> synced slots (slots on the standby that are being synced from the
> primary) can have their own inactive_sicne value. Fundamentally,
> inactive_sicne is set to 0 when slot is acquired and set to current
> time when slot is released, no matter who acquires and releases it -
> be it walsenders for replication, or backends for slot advance, or
> backends for slot sync using pg_sync_replication_slots, or backends
> for other slot functions, or background sync worker. Remember the
> earlier patch was updating inactive_since just for walsenders, but
> then the suggestion was to update it unconditionally -
> https://www.postgresql.org/message-id/CAJpy0uD64X%3D2ENmbHaRiWTKeQawr-rbGoy_GdhQQLVXzUSKTMg%40mail.gmail.com.
> Whoever syncs the slot, *acutally* acquires the slot i.e. makes it
> theirs, syncs it from the primary, and releases it. IMO, no
> differentiation is to be made for synced slots.
> 
> There was a suggestion on using inactive_since of the synced slot on
> the standby to know the inactivity of the slot on the primary. If one
> wants to do that, they better look at/monitor the primary slot
> info/logs/pg_replication_slot/whatever.

Yeah but the use case was in case the primary is down for whatever reason.

> I really don't see a point in
> having two different meanings for a single property of a replication
> slot - inactive_since for a regular slot tells since when this slot
> has become inactive, and for a synced slot since when the
> corresponding remote slot has become inactive. I think this will
> confuse users for sure.

I'm not sure as we are speaking about "synced" slots. I can also see some confusion
if this value is not "synced".

> Also, if inactive_since is being changed on the primary so frequently,
> and none of the other parameters are changing, if we copy
> inactive_since to the synced slots, then standby will just be doing
> *sync* work (mark the slots dirty and save to disk) for updating
> inactive_since. I think this is unnecessary behaviour for sure.

Right, I think we should avoid the save slot to disk in that case (question raised
by Amit in [1]).

> Coming to a future patch for inactive timeout based slot invalidation,
> we can either allow invalidation without any differentiation for
> synced slots or restrict invalidation to avoid more sync work. For
> instance, if inactive timeout is kept low on the standby, the sync
> worker will be doing more work as it drops and recreates a slot
> repeatedly if it keeps getting invalidated. Another thing is that the
> standby takes independent invalidation decisions for synced slots.
> AFAICS, invalidation due to wal_removal is the only sole reason (out
> of all available invalidation reasons) for a synced slot to get
> invalidated independently of the primary. Check
> https://www.postgresql.org/message-id/CAA4eK1JXBwTaDRD_%3D8t6UB1fhRNjC1C%2BgH4YdDxj_9U6djLnXw%40mail.gmail.com
> for the suggestion on we better not differentiaing invalidation
> decisions for synced slots.

Yeah, I think the invalidation decision on the standby is highly linked to
what inactive_since on the standby is: synced from primary or not.

> The assumption of letting synced slots have their own inactive_since
> not only simplifies the code, but also looks less-confusing and more
> meaningful to the user.

I'm not sure at all. But if the majority of us thinks it's the case then let's
go that way.

> > Now, the other concern is that calling GetCurrentTimestamp()
> > could be costly when the values for the slot are not going to be
> > updated but if that happens we can optimize such that before acquiring
> > the slot we can have some minimal pre-checks to ensure whether we need
> > to update the slot or not.

Also maybe we could accept a bit less accuracy and use
GetCurrentTransactionStopTimestamp() instead?

> If we are too much concerned about the cost of GetCurrentTimestamp(),
> a possible approach is just don't set inactive_since for slots being
> synced on the standby.
> Just let the first acquisition and release
> after the promotion do that job. We can always call this out in the
> docs saying "replication slots on the streaming standbys which are
> being synced from the primary are not inactive in practice, so the
> inactive_since is always NULL for them unless the standby is
> promoted".

I think that was the initial behavior that lead to Robert's remark (see [2]):

"
And I'm suspicious that having an exception for slots being synced is
a bad idea. That makes too much of a judgement about how the user will
use this field. It's usually better to just expose the data, and if
the user needs helps to make sense of that data, then give them that
help separately.
"

[1]: https://www.postgresql.org/message-id/CAA4eK1JtKieWMivbswYg5FVVB5FugCftLvQKVsxh%3Dm_8nk04vw%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CA%2BTgmob_Ta-t2ty8QrKHBGnNLrf4ZYcwhGHGFsuUoFrAEDw4sA%40mail.gmail.com

Regards,

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