From 285f87cab5543f34dbcf178aa594193c69666030 Mon Sep 17 00:00:00 2001 From: Nisha Moond Date: Mon, 3 Feb 2025 14:50:55 +0530 Subject: [PATCH v1] Avoid updating inactive_since for invalid replication slots Right now, it is possible for the inactive_since value of an invalid replication slot to be updated multiple times, which is unexpected behavior. This patch introduces a new dedicated function to update the inactive_since for a given replication slot and ensures that inactive_since is not updated for an invalid replication slot. --- doc/src/sgml/system-views.sgml | 3 ++- src/backend/replication/logical/slotsync.c | 4 +--- src/backend/replication/slot.c | 14 +++++++------- src/include/replication/slot.h | 19 +++++++++++++++++++ 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml index 8e2b0a7927..8b04c93e0f 100644 --- a/doc/src/sgml/system-views.sgml +++ b/doc/src/sgml/system-views.sgml @@ -2566,7 +2566,8 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx The time when the slot became inactive. NULL if the - slot is currently being streamed. + slot is currently being streamed. If the slot becomes invalid, + this value will not be updated. Note that for slots on the standby that are being synced from a primary server (whose synced field is true), the inactive_since diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c index be6f87f00b..987857b949 100644 --- a/src/backend/replication/logical/slotsync.c +++ b/src/backend/replication/logical/slotsync.c @@ -1541,9 +1541,7 @@ update_synced_slots_inactive_since(void) if (now == 0) now = GetCurrentTimestamp(); - SpinLockAcquire(&s->mutex); - s->inactive_since = now; - SpinLockRelease(&s->mutex); + ReplicationSlotSetInactiveSince(s, now, true); } } diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index c57a13d820..ee3a4f56a5 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -720,16 +720,12 @@ ReplicationSlotRelease(void) */ SpinLockAcquire(&slot->mutex); slot->active_pid = 0; - slot->inactive_since = now; + ReplicationSlotSetInactiveSince(slot, now, false); SpinLockRelease(&slot->mutex); ConditionVariableBroadcast(&slot->active_cv); } else - { - SpinLockAcquire(&slot->mutex); - slot->inactive_since = now; - SpinLockRelease(&slot->mutex); - } + ReplicationSlotSetInactiveSince(slot, now, true); MyReplicationSlot = NULL; @@ -2218,6 +2214,7 @@ RestoreSlotFromDisk(const char *name) bool restored = false; int readBytes; pg_crc32c checksum; + TimestampTz now = 0; /* no need to lock here, no concurrent access allowed yet */ @@ -2410,7 +2407,10 @@ RestoreSlotFromDisk(const char *name) * slot from the disk into memory. Whoever acquires the slot i.e. * makes the slot active will reset it. */ - slot->inactive_since = GetCurrentTimestamp(); + if (now == 0) + now = GetCurrentTimestamp(); + + ReplicationSlotSetInactiveSince(slot, now, false); restored = true; break; diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index 47ebdaecb6..fc8fe6922e 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -228,6 +228,25 @@ typedef struct ReplicationSlotCtlData ReplicationSlot replication_slots[1]; } ReplicationSlotCtlData; +/* + * Set slot's inactive_since property unless it was previously invalidated. + */ +static inline void +ReplicationSlotSetInactiveSince(ReplicationSlot *s, TimestampTz now, + bool acquire_lock) +{ + if (s->data.invalidated != RS_INVAL_NONE) + return; + + if (acquire_lock) + SpinLockAcquire(&s->mutex); + + s->inactive_since = now; + + if (acquire_lock) + SpinLockRelease(&s->mutex); +} + /* * Pointers to shared memory */ -- 2.34.1