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

Hi,

Thanks for looking into this.

On Fri, Aug 30, 2024 at 8:13 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> ======
> Commit message
>
> 1.
> ... introduces a GUC allowing users set inactive timeout.
>
> ~
>
> 1a. You should give the name of the new GUC in the commit message.

Modified.

> 1b. /set/to set/

Reworded the commit message.

> ======
> doc/src/sgml/config.sgml
>
> GUC "replication_slot_inactive_timeout"
>
> 2.
> Invalidates replication slots that are inactive for longer than
> specified amount of time
>
> nit - suggest use similar wording as the prior GUC (wal_sender_timeout):
> Invalidate replication slots that are inactive for longer than this
> amount of time.

Modified.

> 3.
> This invalidation check happens either when the slot is acquired for
> use or during a checkpoint. The time since the slot has become
> inactive is known from its inactive_since value using which the
> timeout is measured.
>
> nit - the wording is too complicated. suggest:
> The timeout check occurs when the slot is next acquired for use, or
> during a checkpoint. The slot's 'inactive_since' field value is when
> the slot became inactive.

> 4.
> Note that the inactive timeout invalidation mechanism is not
> applicable for slots on the standby that are being synced from a
> primary server (whose synced field is true).
>
> nit - that word "whose" seems ambiguous. suggest:
> (e.g. the standby slot has 'synced' field true).

Reworded.

> ======
> doc/src/sgml/system-views.sgml
>
> 5.
> inactive_timeout means that the slot has been inactive for the
> duration specified by replication_slot_inactive_timeout parameter.
>
> nit - suggestion ("longer than"):
> ... the slot has been inactive for longer than the duration specified
> by the replication_slot_inactive_timeout parameter.

Modified.

> ======
> src/backend/replication/slot.c
>
> 6.
> /* Maximum number of invalidation causes */
> -#define RS_INVAL_MAX_CAUSES RS_INVAL_WAL_LEVEL
> +#define RS_INVAL_MAX_CAUSES RS_INVAL_INACTIVE_TIMEOUT
>
> IMO this #define belongs in the slot.h, immediately below where the
> enum is defined.

Please check the commit that introduced it -
https://www.postgresql.org/message-id/ZdU3CHqza9XJw4P-%40paquier.xyz.
It is kept in the file where it's used.

> 7. ReplicationSlotAcquire:
>
> I had a fundamental question about this logic.
>
> IIUC the purpose of the patch was to invalidate replication slots that
> have been inactive for too long.
>
> So, it makes sense to me that some periodic processing (e.g.
> CheckPointReplicationSlots) might do a sweep over all the slots, and
> invalidate the too-long-inactive ones that it finds.
>
> OTOH, it seemed quite strange to me that the patch logic is also
> detecting and invalidating inactive slots during the
> ReplicationSlotAcquire function. This is kind of saying "ERROR -
> sorry, because this was inactive for too long you can't have it" at
> the very moment that you wanted to use it again! IIUC such a slot
> would be invalidated by the function InvalidatePossiblyObsoleteSlot(),
> but therein lies my doubt -- how can the slot be considered as
> "obsolete" when we are in the very act of trying to acquire/use it?
>
> I guess it might be argued this is not so different to the scenario of
> attempting to acquire a slot that had been invalidated momentarily
> before during checkpoint processing. But, somehow that scenario seems
> more like bad luck to me, versus ReplicationSlotAcquire() deliberately
> invalidating something we *know* is wanted.

Hm. TBH, there's no real reason for invalidating the slot in
ReplicationSlotAcquire(). My thinking back then was to take this
opportunity to do some work. I agree to leave the invalidation work to
the checkpointer. However, I still think ReplicationSlotAcquire()
should error out if the slot has already been invalidated similar to
"can no longer get changes from replication slot \"%s\" for
wal_removed.

> 8.
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("can no longer get changes from replication slot \"%s\"",
> + NameStr(s->data.name)),
> + errdetail("This slot has been invalidated because it was inactive
> since %s for more than %d seconds specified by
> \"replication_slot_inactive_timeout\".",
> + timestamptz_to_str(s->inactive_since),
> + replication_slot_inactive_timeout)));
>
> nit - IMO the info should be split into errdetail + errhint. Like this:
> errdetail("The slot became invalid because it was inactive since %s,
> which is more than %d seconds ago."...)
> errhint("You might need to increase \"%s\".",
> "replication_slot_inactive_timeout")

"invalid" is being covered by errmsg "invalidating obsolete
replication slot", so no need to duplicate it in errdetail.

> 9. ReportSlotInvalidation
>
> + appendStringInfo(&err_detail,
> + _("The slot has been inactive since %s for more than %d seconds
> specified by \"replication_slot_inactive_timeout\"."),
> + timestamptz_to_str(inactive_since),
> + replication_slot_inactive_timeout);
> + break;
>
> IMO this error in ReportSlotInvalidation() should be the same as the
> other one from ReplicationSlotAcquire(), which I suggested above
> (comment #8) should include a hint. Also, including a hint here will
> make this new message consistent with the other errhint (for
> "max_slot_wal_keep_size") that is already in this function.

Not exactly the same but similar. Because ReportSlotInvalidation()
errmsg has an "invalidating" component, whereas errmsg in
ReplicationSlotAcquire doesn't. Please check latest wordings.

> 10. InvalidatePossiblyObsoleteSlot
>
> + if (cause == RS_INVAL_INACTIVE_TIMEOUT &&
> + (replication_slot_inactive_timeout > 0 &&
> + s->inactive_since > 0 &&
> + !(RecoveryInProgress() && s->data.synced)))
>
> 10a. Everything here is && so this has some redundant parentheses.

Removed.

> 10b. Actually, IMO this complicated condition is overkill. Won't it be
> better to just unconditionally assign
> now = GetCurrentTimestamp(); here?

GetCurrentTimestamp() can get costlier on certain platforms. I think
the fields checking in the condition are pretty straight forward -
e.g. !RecoveryInProgress() server not in recovery, !s->data.synced
slot is not being synced and so on. Added a macro
IsInactiveTimeoutSlotInvalidationApplicable() for better readability
in two places.

> 11.
> + * Note that we don't invalidate synced slots because,
> + * they are typically considered not active as they don't
> + * perform logical decoding to produce the changes.
>
> nit - tweaked punctuation

Used the consistent wording in the commit message, docs and code comments.

> 12.
> + * If the slot can be acquired, do so or if the slot is already ours,
> + * then mark it invalidated. Otherwise we'll signal the owning
> + * process, below, and retry.
>
> nit - tidied this comment. Suggestion:
> If the slot can be acquired, do so and mark it as invalidated. If the
> slot is already ours, mark it as invalidated. Otherwise, we'll signal
> the owning process below and retry.

Modified.

> 13.
> + if (active_pid == 0 ||
> + (MyReplicationSlot != NULL &&
> + MyReplicationSlot == s &&
> + active_pid == MyProcPid))
>
> You are already checking MyReplicationSlot == s here, so that extra
> check for MyReplicationSlot != NULL is redundant, isn't it?

Removed.

> 14. CheckPointReplicationSlots
>
> /*
> - * Flush all replication slots to disk.
> + * Flush all replication slots to disk. Also, invalidate slots during
> + * non-shutdown checkpoint.
> *
> * It is convenient to flush dirty replication slots at the time of checkpoint.
> * Additionally, in case of a shutdown checkpoint, we also identify the slots
>
> nit - /Also, invalidate slots/Also, invalidate obsolete slots/

Modified.

> 15.
> + {"replication_slot_inactive_timeout", PGC_SIGHUP, REPLICATION_SENDING,
> + gettext_noop("Sets the amount of time to wait before invalidating an "
> + "inactive replication slot."),
>
> nit - that is maybe a bit misleading because IIUC there is no real
> "waiting" happening anywhere. Suggest:
> Sets the amount of time a replication slot can remain inactive before
> it will be invalidated.

Modified.

Please find the attached v44 patch with the above changes. I will
include the 0002 xid_age based invalidation patch later.

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