From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Tom Lane <tgl@sss.pgh.pa.us>
To: David G. Johnston <david.g.johnston@gmail.com>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Nisha Moond <nisha.moond412@gmail.com>
Cc: Gunnar Morling <gunnar.morling@googlemail.com>
Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Subject: Re: Unexpected behavior when setting "idle_replication_slot_timeout"
Date: Tue, 08 Jul 2025 18:53:25 +0200
Message-ID: <f12271e6aea3b87eaa430d0c9008ca1f3154c31e.camel@cybertec.at> (raw)
In-Reply-To: <474268.1751731988@sss.pgh.pa.us>
References: <CADGJaX_0+FTguWpNSpgVWYQP_7MhoO0D8=cp4XozSQgaZ40Odw@mail.gmail.com>
<CABdArM76e8-XChodqKxEiRK3tR_Yv3cymn_AWfP4bOsLDscZAg@mail.gmail.com>
<30e91649-06dd-453e-8658-5f4acd03a3bd@oss.nttdata.com>
<b26f13d7-769a-4b64-8bed-cb3a833c0604@oss.nttdata.com>
<686577f2379e4b978803e72746dcf0dd7baf3937.camel@cybertec.at>
<e0789f10-5b48-4d6b-a32c-0f4426d254df@oss.nttdata.com>
<CAA4eK1LOyUeCHk3hMtjT-bpqXLAnjODwcO+o7+bTnB2es-fSqw@mail.gmail.com>
<70b183fb-f66e-40d8-b0de-be8c1052fa93@oss.nttdata.com>
<CAKFQuwYVEgy+fiHMvSoykiEef-VF-ZV9Gxv4rynk9NNK9UMPYQ@mail.gmail.com>
<472687.1751731008@sss.pgh.pa.us>
<474268.1751731988@sss.pgh.pa.us>
On Sat, 2025-07-05 at 12:13 -0400, Tom Lane wrote:
> Oh, wait a second. I just noticed that this variable is new in v18.
> So there won't be a compatibility issue as long as we change it in
> v18 too. So changing it to a base of seconds seems sufficient,
> and more in line with existing practice:
>
> Using 'ms' seems clearly overkill, but there's precedent for
> timeouts measured in seconds.
Here is a patch that changes the unit to seconds, like Fujii Masao's
patch upthread. In addition, it removes the now unnecessary injection
point for the TAP test and tries to improve the documentation to
address my complaints.
Yours,
Laurenz Albe
Attachments:
[text/x-patch] v2-0001-Change-idle_replication_slot_timeout-unit-to-seco.patch (10.3K, ../f12271e6aea3b87eaa430d0c9008ca1f3154c31e.camel@cybertec.at/2-v2-0001-Change-idle_replication_slot_timeout-unit-to-seco.patch)
download | inline diff:
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.sgmlindex 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.sgmlindex 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.cindex 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.cindex 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.sampleindex 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.hindex 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.plindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: laurenz.albe@cybertec.at, tgl@sss.pgh.pa.us, david.g.johnston@gmail.com, masao.fujii@oss.nttdata.com, amit.kapila16@gmail.com, nisha.moond412@gmail.com, gunnar.morling@googlemail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: Unexpected behavior when setting "idle_replication_slot_timeout"
In-Reply-To: <f12271e6aea3b87eaa430d0c9008ca1f3154c31e.camel@cybertec.at>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox