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
