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

Hi Nisha,

Here are some review comments for the patch v57-0001.

======
src/backend/replication/slot.c

1.
+/*
+ * Invalidate replication slots that have remained idle longer than this
+ * duration; '0' disables it.
+ */
+int idle_replication_slot_timeout_min = HOURS_PER_DAY * MINS_PER_HOUR;

IMO it would be better to have the suffix "_mins" instead of "_min"
here to avoid any confusion with "minimum".

~~~

2.
+/*
+ * Can invalidate an idle replication slot?
+ *

Not an English sentence.

======
src/backend/utils/adt/timestamp.c

3.
+ /* Return if the difference meets or exceeds the threshold */
+ return (secs >= threshold_sec);

That comment may not be necessary; it is saying just the same as the code.

======
src/backend/utils/misc/guc_tables.c

4.
+ {
+ {"idle_replication_slot_timeout", PGC_SIGHUP, REPLICATION_SENDING,
+ gettext_noop("Sets the duration a replication slot can remain idle before "
+ "it is invalidated."),
+ NULL,
+ GUC_UNIT_MIN
+ },
+ &idle_replication_slot_timeout_min,
+ HOURS_PER_DAY * MINS_PER_HOUR, 0, INT_MAX / SECS_PER_MINUTE,
+ check_idle_replication_slot_timeout, NULL, NULL
+ },
+

Maybe it's better to include a comment that says "24 hours".

(e.g. like wal_summary_keep_time does)

======
src/backend/utils/misc/postgresql.conf.sample

5.
#track_commit_timestamp = off # collect timestamp of transaction commit
# (change requires restart)
+#idle_replication_slot_timeout = 1d # in minutes; 0 disables

I felt it might be better to say 24h here instead of 1d. And, that
would also be consistent with the docs, which said the default was 24
hours.

======
Kind Regards,
Peter Smith.
Fujitsu Australia