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
