v2-0001-Change-idle_replication_slot_timeout-unit-to-seco.patch

From acc88781735355d41e905a2d551a38dc8478ea53 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Tue, 8 Jul 2025 18:47:56 +0200
Subject: [PATCH v2] Change idle_replication_slot_timeout unit to seconds

While there is nothing fundamentally wrong with the original unit
of minutes, having a precision of seconds has some advantages:

First, it makes testing easier.  We don't need an injection point to
test the parameter in a TAP test, since we can afford to wait for two
seconds for replication slots to become invalid.  Also, the original
complaint that led to this patch was that the timeout was rounded to
minutes (as it should be), so values below 30 seconds resulted in an
effective setting of 0, which disabled the timeout to surprise of the
testers.

While at it, improve the documentation to clarify that "idle" in this
context is the same as "inactive" (no replication connection is
currently using the slot).  Otherwise, "idle" might be misunderstood
as "the replication slot is not advanced" or "nothing is streamed on
the replication connection".

Reported-by: Gunnar Morling <gunnar.morling@googlemail.com>
Author: Fujii Masao <masao.fujii@oss.nttdata.com>
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CADGJaX_0%2BFTguWpNSpgVWYQP_7MhoO0D8%3Dcp4XozSQgaZ40Odw%40mail.gmail.com
---
 doc/src/sgml/config.sgml                      |  7 +++--
 doc/src/sgml/system-views.sgml                |  2 +-
 src/backend/replication/slot.c                | 30 +++++--------------
 src/backend/utils/misc/guc_tables.c           |  6 ++--
 src/backend/utils/misc/postgresql.conf.sample |  2 +-
 src/include/replication/slot.h                |  2 +-
 .../t/044_invalidate_inactive_slots.pl        | 27 ++---------------
 7 files changed, 20 insertions(+), 56 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 59a0874528a..09d33069385 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4618,9 +4618,10 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
       </term>
       <listitem>
        <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 (the default) disables the idle timeout
+        Invalidate replication slots that have remained inactive (not used by a
+        <link linkend="protocol-replication">replication connection</link>) for
+        longer than this duration. If this value is specified without units, it
+        is taken as seconds. A value of zero (the 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.
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 986ae1f543d..89f20d4ce6e 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2932,7 +2932,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
         <listitem>
          <para>
           <literal>idle_timeout</literal> means that the slot has remained
-          idle longer than the configured
+          inactive longer than the configured
           <xref linkend="guc-idle-replication-slot-timeout"/> duration.
          </para>
         </listitem>
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index f9fec50ae88..cc13cbb2d9a 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -154,7 +154,7 @@ int			max_replication_slots = 10; /* the maximum number of replication
  * Invalidate replication slots that have remained idle longer than this
  * duration; '0' disables it.
  */
-int			idle_replication_slot_timeout_mins = 0;
+int			idle_replication_slot_timeout_secs = 0;
 
 /*
  * This GUC lists streaming replication standby server slot names that
@@ -1612,13 +1612,10 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause,
 
 		case RS_INVAL_IDLE_TIMEOUT:
 			{
-				int			minutes = slot_idle_seconds / SECS_PER_MINUTE;
-				int			secs = slot_idle_seconds % SECS_PER_MINUTE;
-
 				/* translator: %s is a GUC variable name */
-				appendStringInfo(&err_detail, _("The slot's idle time of %dmin %02ds exceeds the configured \"%s\" duration of %dmin."),
-								 minutes, secs, "idle_replication_slot_timeout",
-								 idle_replication_slot_timeout_mins);
+				appendStringInfo(&err_detail, _("The slot's idle time of %lds exceeds the configured \"%s\" duration of %ds."),
+								 slot_idle_seconds, "idle_replication_slot_timeout",
+								 idle_replication_slot_timeout_secs);
 				/* translator: %s is a GUC variable name */
 				appendStringInfo(&err_hint, _("You might need to increase \"%s\"."),
 								 "idle_replication_slot_timeout");
@@ -1656,7 +1653,7 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause,
 static inline bool
 CanInvalidateIdleSlot(ReplicationSlot *s)
 {
-	return (idle_replication_slot_timeout_mins != 0 &&
+	return (idle_replication_slot_timeout_secs != 0 &&
 			!XLogRecPtrIsInvalid(s->data.restart_lsn) &&
 			s->inactive_since > 0 &&
 			!(RecoveryInProgress() && s->data.synced));
@@ -1716,25 +1713,12 @@ DetermineSlotInvalidationCause(uint32 possible_causes, ReplicationSlot *s,
 
 		if (CanInvalidateIdleSlot(s))
 		{
-			/*
-			 * We simulate the invalidation due to idle_timeout as the minimum
-			 * time idle time is one minute which makes tests take a long
-			 * time.
-			 */
-#ifdef USE_INJECTION_POINTS
-			if (IS_INJECTION_POINT_ATTACHED("slot-timeout-inval"))
-			{
-				*inactive_since = 0;	/* since the beginning of time */
-				return RS_INVAL_IDLE_TIMEOUT;
-			}
-#endif
-
 			/*
 			 * Check if the slot needs to be invalidated due to
 			 * idle_replication_slot_timeout GUC.
 			 */
 			if (TimestampDifferenceExceedsSeconds(s->inactive_since, now,
-												  idle_replication_slot_timeout_mins * SECS_PER_MINUTE))
+												  idle_replication_slot_timeout_secs))
 			{
 				*inactive_since = s->inactive_since;
 				return RS_INVAL_IDLE_TIMEOUT;
@@ -1895,7 +1879,7 @@ InvalidatePossiblyObsoleteSlot(uint32 possible_causes,
 		 * max_slot_wal_keep_size is set to -1 and
 		 * idle_replication_slot_timeout is set to 0 during the binary
 		 * upgrade. See check_old_cluster_for_valid_slots() where we ensure
-		 * that no invalidated before the upgrade.
+		 * that no slot was invalidated before the upgrade.
 		 */
 		Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));
 
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 511dc32d519..a925be86944 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -3100,10 +3100,10 @@ struct config_int ConfigureNamesInt[] =
 			gettext_noop("Sets the duration a replication slot can remain idle before "
 						 "it is invalidated."),
 			NULL,
-			GUC_UNIT_MIN
+			GUC_UNIT_S
 		},
-		&idle_replication_slot_timeout_mins,
-		0, 0, INT_MAX / SECS_PER_MINUTE,
+		&idle_replication_slot_timeout_secs,
+		0, 0, INT_MAX,
 		check_idle_replication_slot_timeout, NULL, NULL
 	},
 
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 341f88adc87..a9d8293474a 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -342,7 +342,7 @@
 				# (change requires restart)
 #wal_keep_size = 0		# in megabytes; 0 disables
 #max_slot_wal_keep_size = -1	# in megabytes; -1 disables
-#idle_replication_slot_timeout = 0	# in minutes; 0 disables
+#idle_replication_slot_timeout = 0	# in seconds; 0 disables
 #wal_sender_timeout = 60s	# in milliseconds; 0 disables
 #track_commit_timestamp = off	# collect timestamp of transaction commit
 				# (change requires restart)
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index ffacba9d2ae..76aeeb92242 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -266,7 +266,7 @@ extern PGDLLIMPORT ReplicationSlot *MyReplicationSlot;
 /* GUCs */
 extern PGDLLIMPORT int max_replication_slots;
 extern PGDLLIMPORT char *synchronized_standby_slots;
-extern PGDLLIMPORT int idle_replication_slot_timeout_mins;
+extern PGDLLIMPORT int idle_replication_slot_timeout_secs;
 
 /* shmem initialization functions */
 extern Size ReplicationSlotsShmemSize(void);
diff --git a/src/test/recovery/t/044_invalidate_inactive_slots.pl b/src/test/recovery/t/044_invalidate_inactive_slots.pl
index ccace14b4dd..4abcc960f19 100644
--- a/src/test/recovery/t/044_invalidate_inactive_slots.pl
+++ b/src/test/recovery/t/044_invalidate_inactive_slots.pl
@@ -8,14 +8,6 @@ use PostgreSQL::Test::Utils;
 use PostgreSQL::Test::Cluster;
 use Test::More;
 
-# This test depends on injection point that forces slot invalidation
-# due to idle_timeout.
-# https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-ADDIN-INJECTION-POINTS
-if ($ENV{enable_injection_points} ne 'yes')
-{
-	plan skip_all => 'Injection points not supported by this build';
-}
-
 # Wait for slot to first become idle and then get invalidated
 sub wait_for_slot_invalidation
 {
@@ -51,18 +43,10 @@ $node->init(allows_streaming => 'logical');
 $node->append_conf(
 	'postgresql.conf', qq{
 checkpoint_timeout = 1h
-idle_replication_slot_timeout = 1min
+idle_replication_slot_timeout = 1s
 });
 $node->start;
 
-# Check if the 'injection_points' extension is available, as it may be
-# possible that this script is run with installcheck, where the module
-# would not be installed by default.
-if (!$node->check_extension('injection_points'))
-{
-	plan skip_all => 'Extension injection_points not installed';
-}
-
 # Create both physical and logical replication slots
 $node->safe_psql(
 	'postgres', qq[
@@ -72,13 +56,8 @@ $node->safe_psql(
 
 my $log_offset = -s $node->logfile;
 
-# Register an injection point on the node to forcibly cause a slot
-# invalidation due to idle_timeout
-$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
-
-$node->safe_psql('postgres',
-	"SELECT injection_points_attach('slot-timeout-inval', 'error');");
-
+# Wait a bit for the replication slots to become invalid
+$node->safe_psql('postgres', "SELECT pg_sleep(2)");
 # Slot invalidation occurs during a checkpoint, so perform a checkpoint to
 # invalidate the slots.
 $node->safe_psql('postgres', "CHECKPOINT");
-- 
2.50.0