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

On Thu, 2 Jan 2025 at 15:57, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Thu, Jan 2, 2025 at 8:16 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Nisha,
> >
> > Here are some minor review comments for patch v58-0002.
> >
>
> Thank you for your feedback! Please find the v59 patch set addressing
> all the comments.
> Note: There are no new changes in patch-0001.

Few comments:
1) I felt we should not invalidate the slots for which has no
effective xmin set as they will not be holding the WAL files from
deletion. This can happen when user created slots with
immediately_reserve as false and lsn will be actually reserved only
after the first connection from a streaming replication client:
+static inline bool
+CanInvalidateIdleSlot(ReplicationSlot *s)
+{
+ return (idle_replication_slot_timeout_mins > 0 &&
+ s->inactive_since > 0 &&
+ !(RecoveryInProgress() && s->data.synced));
+}

2) We can mention this as 1d instead of 24h as we want to represent 1
day similar to how we have mentioned for log_rotation_age:
index a2ac7575ca..7284edfbc1 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -337,6 +337,7 @@
#wal_sender_timeout = 60s # in milliseconds; 0 disables
#track_commit_timestamp = off # collect timestamp of transaction commit
# (change requires restart)
+#idle_replication_slot_timeout = 24h # in minutes; 0 disables

Regards,
Vignesh