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

Review comments for v69-0001.

======
doc/src/sgml/config.sgml

1.
+ <para>
+ Invalidate replication slots that have remained idle longer than this
+ duration. If this value is specified without units, it is taken as
+ minutes. A value of zero (which is default) disables the idle timeout
+ invalidation mechanism. This parameter can only be set in the
+ <filename>postgresql.conf</filename> file or on the server command
+ line.
+ </para>

Suggest writing "(the default)" instead of "(which is default)" to be
consistent with the wording of other descriptions on this page.

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

2.
+static inline bool
+CanInvalidateIdleSlot(ReplicationSlot *s)
+{
+ return (idle_replication_slot_timeout_mins > 0 &&
+ !XLogRecPtrIsInvalid(s->data.restart_lsn) &&
+ s->inactive_since > 0 &&
+ !(RecoveryInProgress() && s->data.synced));
}

I wasn't sure why those conditions were '> 0' instead of just '!= 0'.
IIUC negative values aren't possible for
idle_replication_slot_timeout_mins and active_since anyhow.

But, the current patch code is also ok if you prefer.

~~~

3.
+ if (cause == RS_INVAL_IDLE_TIMEOUT)
+ {
+ /*
+ * We get the current time beforehand to avoid system call while
+ * holding the spinlock.
+ */
+ now = GetCurrentTimestamp();
+ }
+

SUGGESTION
Assign the current time here to reduce system call overhead while
holding the spinlock in subsequent code.

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