On Thu, Feb 6, 2025 at 8:02 AM Nisha Moond <nisha.moond...@gmail.com> wrote:
>
> On Wed, Feb 5, 2025 at 2:42 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > Would it address your concern if we write the actual idle duration
> > (now - inactive_since) instead of directly using inactive_since in the
> > above message?
> >
>
> Simply using the raw timestamp difference (now - inactive_since) would
> look odd. We should convert it into a user-friendly format. Since
> idle_replication_slot_timeout is in minutes, we can express the
> difference in minutes and seconds in the log.
> For example:
> DETAIL: The slot's idle time of 1 minute and 7 seconds exceeds the
> configured "idle_replication_slot_timeout" duration.
>
This is better but the implementation should be done on the caller
side mainly because we don't want to call a new GetCurrentTimestamp()
in ReportSlotInvalidation.
>
> > 2.
> > + * Flush all replication slots to disk. Also, invalidate obsolete 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
> > @@ -1924,6 +2007,45 @@ CheckPointReplicationSlots(bool is_shutdown)
> >
> > Can we try and see how the patch looks if we try to invalidate the
> > slot due to idle time at the same time when we are trying to
> > invalidate due to WAL?
> >
>
> I'll consider the suggested change in the next version.
>
FYI, we discussed this previously (1), but the conclusion that it
won't help much (as it will not help to remove WAL immediately) is
incorrect, especially if we do what is suggested now.
Apart from this, I have made minor changes in the comments. Please
review and include them unless you disagree.
(1) -
https://www.postgresql.org/message-id/CALj2ACXe8%2BxSNdMXTMaSRWUwX7v61Ad4iddUwnn%3DdjSwx3GLLg%40mail.gmail.com
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 29749ce917..4eb679e187 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1598,11 +1598,11 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause
cause,
* Idle timeout invalidation is allowed only when:
*
* 1. Idle timeout is set
- * 2. Slot has WAL reserved
+ * 2. Slot has reserved WAL
* 3. Slot is inactive
- * 4. The slot is not being synced from the primary while the server
- * is in recovery. As synced slots are always considered to be inactive
- * because they don't perform logical decoding to produce changes.
+ * 4. The slot is not being synced from the primary while the server is in
+ * recovery. This is because synced slots are always considered to be
+ * inactive because they don't perform logical decoding to produce
changes.
*/
static inline bool
CanInvalidateIdleSlot(ReplicationSlot *s)
@@ -1662,7 +1662,7 @@
InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
if (cause == RS_INVAL_IDLE_TIMEOUT)
{
/*
- * Assign the current time here to reduce system call
overhead
+ * Assign the current time here to avoid system call
overhead
* while holding the spinlock in subsequent code.
*/
now = GetCurrentTimestamp();
