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

On Mon, Apr 1, 2024 at 12:18 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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.

FWIW, coming to this thread late, I think that the inactive_since
should not be synchronized from the primary. The wall clocks are
different on the primary and the standby so having the primary's
timestamp on the standby can confuse users, especially when there is a
big clock drift. Also, as Amit mentioned, inactive_since seems not to
be consistent with other parameters we copy. The
replication_slot_inactive_timeout feature should work on the standby
independent from the primary, like other slot invalidation mechanisms,
and it should be based on its own local clock.

> 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.
>
> 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. The only code that we put in on top of the
> committed code is to use InRecovery in place of
> RecoveryInProgress() in RestoreSlotFromDisk() to fix the issue raised
> by Shveta upthread.

If we want to invalidate the synced slots due to the timeout, I think
we need to define what is "inactive" for synced slots.

Suppose that the slotsync worker updates the local (synced) slot's
inactive_since whenever releasing the slot, irrespective of the actual
LSNs (or other slot parameters) having been updated. I think that this
idea cannot handle a slot that is not acquired on the primary. In this
case, the remote slot is inactive but the local slot is regarded as
active.  WAL files are piled up on the standby (and on the primary) as
the slot's LSNs don't move forward. I think we want to regard such a
slot as "inactive" also on the standby and invalidate it because of
the timeout.

>
> > 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.

If we use such pre-checks, another problem might happen; it cannot
handle a case where the slot is acquired on the primary but its LSNs
don't move forward. Imagine a logical replication conflict happened on
the subscriber, and the logical replication enters the retry loop. In
this case, the remote slot's inactive_since gets updated for every
retry, but it looks inactive from the standby since the slot LSNs
don't change. Therefore, only the local slot could be invalidated due
to the timeout but probably we don't want to regard such a slot as
"inactive".

Another idea I came up with is that the slotsync worker updates the
local slot's inactive_since to the local timestamp only when the
remote slot might have got inactive. If the remote slot is acquired by
someone, the local slot's inactive_since is also NULL. If the remote
slot gets inactive, the slotsync worker sets the local timestamp to
the local slot's inactive_since. Since the remote slot could be
acquired and released before the slotsync worker gets the remote slot
data again, if the remote slot's inactive_since > the local slot's
inactive_since, the slotsync worker updates the local one. IOW, we
detect whether the remote slot was acquired and released since the
last synchronization, by checking the remote slot's inactive_since.
This idea seems to handle these cases I mentioned unless I'm missing
something, but it requires for the slotsync worker to update
inactive_since in a different way than other parameters.

Or a simple solution is that the slotsync worker updates
inactive_since as it does for non-synced slots, and disables
timeout-based slot invalidation for synced slots.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com