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

Hi Nisha,

Here are some minor review comments for patch v58-0002.

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

check_replication_slot_inactive_timeout:

1.
+
+/*
+ * GUC check_hook for idle_replication_slot_timeout
+ *
+ * We don't allow the value of idle_replication_slot_timeout other
+ * than 0 during the binary upgrade.
+ * See start_postmaster() in pg_upgrade for more details.
+ */

If you want to express it this way, then it seems there are some
wrong/missing words:

SUGGESTION #1.
We don't allow any value of idle_replication_slot_timeout other than 0
during a binary upgrade.

SUGGESTION #2.
We don't allow the value of idle_replication_slot_timeout to be other
than 0 during a binary upgrade.

~

(But, I prefer more terse comments which are not negative-sounding. YMMV).

SUGGESTION #3 (nearly identical text to the actual error message)
The value of idle_replication_slot_timeout must be set to 0 during a
binary upgrade.

======
src/test/recovery/README

2.
+If you want to test idle_replication_slot_timeout, add
+PG_TEST_EXTRA=idle_replication_slot_timeout
+to the "make" command. The test takes over 2 minutes, so not done
+by default.
+

Maybe it's better to use consistent wording with the other tests like
this one already in the README:

/The test/This test/

/so not done by default./so it's not done by default./

======
.../t/043_invalidate_inactive_slots.pl

3.
+# Copyright (c) 2024, PostgreSQL Global Development Group
+

Happy New Year.

/2024/2025/

~~~

4.
+
+# The test takes over two minutes to complete. Run it only if
+# idle_replication_slot_timeout is specified in PG_TEST_EXTRA.
+if ( !$ENV{PG_TEST_EXTRA}
+ || $ENV{PG_TEST_EXTRA} !~ /\bidle_replication_slot_timeout\b/)
+{
+ plan skip_all =>
+ 'A time consuming test, idle_replication_slot_timeout is not
enabled in PG_TEST_EXTRA';
+}

4a.
I noticed the other skipping TAP tests like this have a simpler
message without giving a reason, so maybe it's better to be consistent
with those:

SUGGESTION:
plan skip_all => "test idle_replication_slot_timeout not enabled in
PG_TEST_EXTRA";

~

4b.
Should the check be done right at the top of the file (e.g. even
before the "# Testcase start" comment)?

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