On Thu, Feb 6, 2025 at 10:17 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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. >
Done. > > > > > 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. > > > Done the changes as suggested in v71. > 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. > Done. ~~~~ Here are the v71 patches with the above comments incorporated. -- Thanks, Nisha
v71-0001-Introduce-inactive_timeout-based-replication-slo.patch
Description: Binary data
v71-0002-Add-TAP-test-for-slot-invalidation-based-on-inac.patch
Description: Binary data
