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

From: Nisha Moond <[email-masked]>
Date: Fri, 13 Dec 2024 16:28:58 +0530

On Thu, Dec 12, 2024 at 9:42 AM vignesh C <[email-masked]> wrote:
>
>
> Now that we support idle_replication_slot_timeout in milliseconds, we
> can set this value from 1s to 1ms or 10millseconds and change sleep to
> usleep, this will bring down the test execution time significantly:

+1
v55 implements the test using idle_replication_slot_timeout=1ms,
significantly reducing the test time.

Attached the v55 patch set which addresses all the comments in [1], [2] as well.

[1] https://www.postgresql.org/message-id/CAHut%2BPvx294U-XBB6-BvabesUNxbnuDQmk-VOFm%3DpbcNWSsHvQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALDaNm2wHDnboo0FCj247HiBMHAHqy0se8NTH4fDCdscxdjhcg%40mail.gmail.com

--
Thanks,
Nisha

---

From: Peter Smith <[email-masked]>
Date: Mon, 16 Dec 2024 15:28:24 +1100

Hi Nisha.

Thanks for the v55* patches.

I have no comments for patch v55-0001.

I have only 1 comment for patch v55-0002 regarding some remaining
nitpicks (below) about the consistency of phrases.

======

I scanned again over all the phrases for consistency:

CURRENT PATCH:

Docs (idle_replication_slot_timeout): Invalidate replication slots
that are idle for longer than this amount of time
Docs (idle_timeout): means that the slot has remained idle longer than
the duration specified by the idle_replication_slot_timeout parameter.

Code (guc var comment):  Invalidate replication slots idle longer than this time
Code (guc_tables): Sets the time limit for how long a replication slot
can remain idle before it is invalidated.

Msg (errdetail): This slot has been invalidated because it has
remained idle longer than the configured \"%s\" time.
Msg (errdetail): The slot has been inactive since %s and has remained
idle longer than the configured \"%s\" time.

~

NITPICKS:

nit -- There are still some variations "amount of time" versus "time"
versus "duration".  I think the term "duration" best describe the
maing so we can use that everywhere.

nit - Should consistently say "remained idle" instead of just "idle"
or "are idle",

nit - The last errdetail is also rearranged a bit because IMO we don't
need to say inactive and idle in the same sentence.

nit - Just say "longer than" instead of sometimes saying "for longer than"

~

SUGGESTIONS:

Docs (idle_replication_slot_timeout): Invalidate replication slots
that have remained idle longer than this duration.
Docs (idle_timeout): means that the slot has remained idle longer than
the configured idle_replication_slot_timeout duration.

Code (guc var comment):  Invalidate replication slots that have
remained idle longer than this duration.
Code (guc_tables): Sets the duration a replication slot can remain
idle before it is invalidated.

Msg (errdetail): This slot has been invalidated because it has
remained idle longer than the configured \"%s\" duration.
Msg (errdetail): The slot has remained idle since %s, which is longer
than the configured \"%s\" duration.

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

---

From: Nisha Moond <[email-masked]>
Date: Mon, 16 Dec 2024 16:10:28 +0530

> than the configured \"%s\" duration.

Here is the v56 patch set with the above comments incorporated.

--
Thanks
Nisha

---

From: Peter Smith <[email-masked]>
Date: Tue, 17 Dec 2024 09:46:49 +1100

> On Mon, Dec 16, 2024 at 9:58 AM Peter Smith <[email-masked]> wrote:
...

Hi Nisha.

Thanks for the updates.

- Both patches could be applied cleanly.
- Tests (make check, TAP subscriber, TAP recovery) are all passing.
- The rendering of the documentation changes from patch 0002 looked good.
- I have no more review comments.

So, the v56* patchset LGTM.

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

---

From: Amit Kapila <[email-masked]>
Date: Fri, 20 Dec 2024 15:12:06 +0530

> Here is the v56 patch set with the above comments incorporated.

Review comments:
===============
1.
+ {
+ {"idle_replication_slot_timeout", PGC_SIGHUP, REPLICATION_SENDING,
+ gettext_noop("Sets the duration a replication slot can remain idle before "
+ "it is invalidated."),
+ NULL,
+ GUC_UNIT_MS
+ },
+ &idle_replication_slot_timeout_ms,

I think users are going to keep idele_slot timeout at least in hours.
So, millisecond seems the wrong choice to me. I suggest to keep the
units in minutes. I understand that writing a test would be
challenging as spending a minute or more on one test is not advisable.
But I don't see any test testing the other GUCs that are in minutes
(wal_summary_keep_time and log_rotation_age). The default value should
be one day.

2.
+ /*
+ * An error is raised if error_if_invalid is true and the slot is found to
+ * be invalid.
+ */
+ if (error_if_invalid && s->data.invalidated != RS_INVAL_NONE)
+ {
+ StringInfoData err_detail;
+
+ initStringInfo(&err_detail);
+
+ switch (s->data.invalidated)
+ {
+ case RS_INVAL_WAL_REMOVED:
+ appendStringInfo(&err_detail, _("This slot has been invalidated
because the required WAL has been removed."));
+ break;
+
+ case RS_INVAL_HORIZON:
+ appendStringInfo(&err_detail, _("This slot has been invalidated
because the required rows have been removed."));
+ break;
+
+ case RS_INVAL_WAL_LEVEL:
+ /* translator: %s is a GUC variable name */
+ appendStringInfo(&err_detail, _("This slot has been invalidated
because \"%s\" is insufficient for slot."),
+ "wal_level");
+ break;
+
+ case RS_INVAL_NONE:
+ pg_unreachable();
+ }
+
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("can no longer get changes from replication slot \"%s\"",
+    NameStr(s->data.name)),
+ errdetail_internal("%s", err_detail.data));
+ }
+

This should be moved to a separate function.

3.
+static inline bool
+IsSlotIdleTimeoutPossible(ReplicationSlot *s)

Would it be better to name this function as CanInvalidateIdleSlot()?
The current name doesn't seem to match with similar other
functionalities.

-- 
With Regards,
Amit Kapila.

---

From: Nisha Moond <[email-masked]>
Date: Tue, 24 Dec 2024 17:06:55 +0530

> > Here is the v56 patch set with the above comments incorporated.

+1
- Changed the GUC unit to "minute".

Regarding the tests, we have two potential options:
 1) Introduce an additional "debug_xx" GUC parameter with units of seconds
or milliseconds, only for testing purposes.
 2) Skip writing tests for this, similar to other GUCs with units in
minutes.

IMO, adding an additional GUC just for testing may not be worthwhile. It's
reasonable to proceed without the test.

Thoughts?

The attached v57 patch-set addresses all the comments. I have kept the test
case in the patch for now, it takes 2-3 minutes to complete.

--
Thanks,
Nisha

---

From: Michail Nikolaev <[email-masked]>
Date: Tue, 24 Dec 2024 13:57:15 +0100

Hello everyone!

Yesterday I got a strange set of test errors, probably somehow related to
that patch.
It happened on changed master branch (based
on d96d1d5152f30d15678e08e75b42756101b7cab6) but I don't think my changes
were affecting it.

My setup is a little bit tricky: Windows 11 run WSL2 with Ubuntu, meson.

So, `recovery ` suite started failing on:

1) at /src/test/recovery/t/019_replslot_limit.pl line 530.
2) at /src/test/recovery/t/040_standby_failover_slots_sync.pl line 198.

It was failing almost every run, one test or another. I was lurking around
for about 10 min, and..... it just stopped failing. And I can't reproduce
it anymore.

But I have logs of two fails. I am not sure if it is helpful, but decided
to mail them here just in case.

Best regards,
Mikhail.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 26 Dec 2024 06:02:20 +0000

On Tuesday, December 24, 2024 8:57 PM Michail Nikolaev <[email-masked]>  wrote:

Hi,

> that patch. It happened on changed master branch (based on
> d96d1d5152f30d15678e08e75b42756101b7cab6) but I don't think my changes were
> affecting it.
> 
> 1) at /src/test/recovery/t/http://019_replslot_limit.pl line 530.
> 2) at /src/test/recovery/t/http://040_standby_failover_slots_sync.pl line
>    198.
> 
> for about 10 min, and..... it just stopped failing. And I can't reproduce it
> anymore.
> 
> But I have logs of two fails. I am not sure if it is helpful, but decided to
> mail them here just in case.

Thanks for reporting the issue.

After checking the log, I think the failure is caused by the unexpected
behavior of the local system clock.

It's clear from the '019_replslot_limit_primary4.log'[1] that the clock went
backwards which makes the slot's inactive_since go backwards as well. That's
why the last testcase didn't pass.

And for 040_standby_failover_slots_sync, we can see that the clock of standby
lags behind that of the primary, which caused the inactive_since of newly synced
slot on standby to be earlier than the one on the primary.

So, I think it's not a bug in the committed patch but an issue in the testing
environment. Besides, since we have not seen such failures on BF, I think it
may not be necessary to improve the testcases.

[1]
2024-12-24 01:37:19.967 CET [161409] sub STATEMENT:  START_REPLICATION SLOT "lsub4_slot" LOGICAL 0/0 (proto_version '4', streaming 'parallel', origin 'any', publication_names '"pub"')
...
2024-12-24 01:37:20.025 CET [161447] 019_replslot_limit.pl LOG:  statement: SELECT '0/30003D8' <= replay_lsn AND state = 'streaming'
...
2024-12-24 01:37:19.388 CET [161097] LOG:  received fast shutdown request

Best Regards,
Hou zj

---

From: Michail Nikolaev <[email-masked]>
Date: Thu, 26 Dec 2024 14:56:50 +0100

Hello, Hou!

>  venvironment. Besides, since we have not seen such failures on BF, I
think it
>  may not be necessary to improve the testcases.

Thanks for your analysis!
Yes, probably WSL2/Windows interactions cause strange system clock moving.
It looks like it is a common issue with WSL2 [0].

[0]: https://github.com/microsoft/WSL/issues/10006

Best regards,
Mikhail.

---

From: vignesh C <[email-masked]>
Date: Fri, 27 Dec 2024 09:22:18 +0530

On Tue, 24 Dec 2024 at 17:07, Nisha Moond <[email-masked]> wrote:
>  1) Introduce an additional "debug_xx" GUC parameter with units of seconds or milliseconds, only for testing purposes.
>  2) Skip writing tests for this, similar to other GUCs with units in minutes.
>
> IMO, adding an additional GUC just for testing may not be worthwhile. It's reasonable to proceed without the test.
>
> Thoughts?
>
> The attached v57 patch-set addresses all the comments. I have kept the test case in the patch for now, it takes 2-3 minutes to complete.

Few comments:
1) We have disabled the similar configuration max_slot_wal_keep_size
by setting to -1, as this GUC also is in similar lines, should we
disable this and let the user configure it?
+/*
+ * Invalidate replication slots that have remained idle longer than this
+ * duration; '0' disables it.
+ */
+int                    idle_replication_slot_timeout_min =
HOURS_PER_DAY * MINS_PER_HOUR;
+

2) I felt this behavior is an existing behavior, so this can also be
moved to 0001 patch:
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index a586156614..199d7248ee 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2566,7 +2566,8 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
       </para>
       <para>
         The time when the slot became inactive. <literal>NULL</literal> if the
-        slot is currently being streamed.
+        slot is currently being streamed. If the slot becomes invalidated,
+        this value will remain unchanged until server shutdown.

3) Can we change the comment below to "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.":
+ * The idle_replication_slot_timeout must be disabled (set to 0)
+ * during the binary upgrade.
+ */
+bool
+check_idle_replication_slot_timeout(int *newval, void **extra,
GucSource source)

Regards,
Vignesh

---

From: Peter Smith <[email-masked]>
Date: Mon, 30 Dec 2024 12:15:19 +1100

Hi Nisha.

Here are some review comments for patch v57-0001.

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

1.
+
+/*
+ * Raise an error based on the invalidation cause of the slot.
+ */
+static void
+RaiseSlotInvalidationError(ReplicationSlot *slot)
+{
+ StringInfoData err_detail;
+
+ initStringInfo(&err_detail);
+
+ switch (slot->data.invalidated)

1a.
/invalidation cause of the slot./slot's invalidation cause./

~

1b.
This function does not expect to be called with slot->data.invalidated
== RS_INVAL_NONE, so I think it will be better to assert that
up-front.

~

1c.
This code could be simplified if you declare/initialize the variable
together, like:

StringInfo err_detail = makeStringInfo();

~~~

2.

Since there are no format strings here, appendStringInfoString can be
used directly in some places.

======

FYI. I've attached a diffs patch that implements some of the
above-suggested changes.

======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 2a99c1f..71c6ae2 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -2816,23 +2816,23 @@ WaitForStandbyConfirmation(XLogRecPtr wait_for_lsn)
 static void
 RaiseSlotInvalidationError(ReplicationSlot *slot)
 {
-	StringInfoData err_detail;
+	StringInfo err_detail = makeStringInfo();
 
-	initStringInfo(&err_detail);
+	Assert(slot->data.invalidated != RS_INVAL_NONE);
 
 	switch (slot->data.invalidated)
 	{
 		case RS_INVAL_WAL_REMOVED:
-			appendStringInfo(&err_detail, _("This slot has been invalidated because the required WAL has been removed."));
+			appendStringInfoString(err_detail, _("This slot has been invalidated because the required WAL has been removed."));
 			break;
 
 		case RS_INVAL_HORIZON:
-			appendStringInfo(&err_detail, _("This slot has been invalidated because the required rows have been removed."));
+			appendStringInfoString(err_detail, _("This slot has been invalidated because the required rows have been removed."));
 			break;
 
 		case RS_INVAL_WAL_LEVEL:
 			/* translator: %s is a GUC variable name */
-			appendStringInfo(&err_detail, _("This slot has been invalidated because \"%s\" is insufficient for slot."),
+			appendStringInfo(err_detail, _("This slot has been invalidated because \"%s\" is insufficient for slot."),
 							 "wal_level");
 			break;
 
@@ -2844,5 +2844,5 @@ RaiseSlotInvalidationError(ReplicationSlot *slot)
 			errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 			errmsg("can no longer get changes from replication slot \"%s\"",
 				   NameStr(slot->data.name)),
-			errdetail_internal("%s", err_detail.data));
+			errdetail_internal("%s", err_detail->data));
 }

---

From: Peter Smith <[email-masked]>
Date: Mon, 30 Dec 2024 16:34:45 +1100

Hi Nisha.

I think we are often too quick to throw out perfectly good tests.
Citing that some similar GUCs don't do testing as a reason to skip
them just seems to me like an example of "two wrongs don't make a
right".

There is a third option.

Keep the tests. Because they take excessive time to run, that simply
means you should run them *conditionally* based on the PG_TEST_EXTRA
environment variable so they don't impact the normal BF execution. The
documentation [1] says this env var is for "resource intensive" tests
-- AFAIK this is exactly the scenario we find ourselves in, so is
exactly what this env var was meant for.

Search other *.pl tests for PG_TEST_EXTRA to see some examples.

======
[1] https://www.postgresql.org/docs/17/regress-run.html

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Tue, 28 Jan 2025 15:26:13 +0530

> Search other *.pl tests for PG_TEST_EXTRA to see some examples.

I don't see the long-running tests to be added under PG_TEST_EXTRA as
that will make it unusable after some point. Now, if multiple senior
members feel it is okay to add long-running tests under PG_TEST_EXTRA
then I am open to considering it. We can keep this test as a separate
patch so that the patch is being tested in CI or in manual tests
before commit.

-- 
With Regards,
Amit Kapila.

---

From: Nisha Moond <[email-masked]>
Date: Tue, 28 Jan 2025 17:28:29 +0530

> patch so that the patch is being tested in CI or in manual tests

Please find the attached v64 patches. The changes in this version
w.r.t. older patch v63 are as -
- The changes from the v63-0001 patch have been moved to a separate thread [1].
- The v63-0002 patch has been split into two parts in v64:
  1) 001 patch: Implements the main feature - inactive timeout-based
slot invalidation.
  2) 002 patch: Separates the TAP test "044_invalidate_inactive_slots"
as suggested above.

[1] https://www.postgresql.org/message-id/CABdArM6pBL5hPnSQ%2B5nEVMANcF4FCH7LQmgskXyiLY75TMnKpw%40mail.gmail.com

--
Thanks,
Nisha

---

From: vignesh C <[email-masked]>
Date: Wed, 29 Jan 2025 11:45:18 +0530

> [1] https://www.postgresql.org/message-id/CABdArM6pBL5hPnSQ%2B5nEVMANcF4FCH7LQmgskXyiLY75TMnKpw%40mail.gmail.com

Few comments:
1) We can mention about the slot that do not reserve WAL is also not applicable:
+       <para>
+        Note that the idle timeout invalidation mechanism is not
+        applicable for slots on the standby server that are being synced
+        from the primary server (i.e., standby slots having
+        <link linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>synced</structfield>
+        value <literal>true</literal>).
+        Synced slots are always considered to be inactive because they don't
+        perform logical decoding to produce changes.
+       </para>

2) Similarly we can mention in the commit message also that it will
not be considered for slot that do not reserve WAL:
Note that the idle timeout invalidation mechanism is not
applicable for slots on the standby server that are being synced
from the primary server (i.e., standby slots having 'synced' field
'true'). Synced slots are always considered to be inactive because
they don't perform logical decoding to produce changes.

3) Since idle_replication_slot_timeout is somewhat similar to
max_slot_wal_keep_size, we can move idle_replication_slot_timeout
after max_slot_wal_keep_size instead of keeping it after
wal_sender_timeout.
+     <varlistentry id="guc-idle-replication-slot-timeout"
xreflabel="idle_replication_slot_timeout">
+      <term><varname>idle_replication_slot_timeout</varname>
(<type>integer</type>)
+      <indexterm>
+       <primary><varname>idle_replication_slot_timeout</varname>
configuration parameter</primary>
+      </indexterm>
+      </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 disables the idle timeout
invalidation mechanism.
+        The default is one day. This parameter can only be set in the
+        <filename>postgresql.conf</filename> file or on the server
command line.
+       </para>

4) We can try to keep it to less than 80 char wherever possible:
a) Like in this case, "mechanism" can be moved to the next line:
+        duration. If this value is specified without units, it is taken as
+        minutes. A value of zero disables the idle timeout
invalidation mechanism.

b) Similarly here too, "slot's" can be moved to the next line:
+        inactive slots. The duration of slot inactivity is calculated
using the slot's
+        <link linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>inactive_since</structfield>

5) You can use new ereport style to exclude brackets around errcode:
+               ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                                errmsg("can no longer get changes
from replication slot \"%s\"",
+                                               NameStr(s->data.name)),
+                                errdetail("This slot has been
invalidated because it has remained idle longer than the configured
\"%s\" duration.",
+
"idle_replication_slot_timeout")));

Regards,
Vignesh

---

From: Peter Smith <[email-masked]>
Date: Wed, 29 Jan 2025 18:01:04 +1100

Hi Nisha.

Some review comments for patch v64-0001.

======
1. General

Too much of this patch v64-0001 is identical/duplicated code with the
recent "spin-off" patch v1-0002 [1]. e.g. Most of v1-0001 is now also
embedded in the v64-0001.

This is making for an unnecessarily tricky 2 x review of all the same
code, and it will also cause rebase hassles later.

Even if you wanted the 'error_in_invalid' stuff to be discussed and
pushed separately, I think it will be much easier to keep a "COPY" of
that v1-0002 patch here as a pre-requisite for v64-0001 so then all of
the current code duplications can be removed.

======

ReplicationSlotAcquire:

2.
+ *
  */

and

+ /*
+ * An error is raised if error_if_invalid is true and the slot has been
+ * previously invalidated due to inactive timeout.
+ */
+ if (error_if_invalid && s->data.invalidated == RS_INVAL_IDLE_TIMEOUT)
+ {

Although those comments are correct for v1-0001 [1] it is a misleading
comment in the hacked into v64-0001 because here you are only checking
invalidation cause RS_INVAL_IDLE_TIMEOUT but none of the other
possible causes.

~~~

ReportSlotInvalidation:

3.
+ case RS_INVAL_IDLE_TIMEOUT:
+ Assert(inactive_since > 0);
+ /* translator: second %s is a GUC variable name */
+ appendStringInfo(&err_detail,
+ _("The slot has remained idle since %s, which is longer than the
configured \"%s\" duration."),
+ timestamptz_to_str(inactive_since),
+ "idle_replication_slot_timeout");

I have the same question already asked for my review of patch v1-0002
[1]. e.g. Isn't there some mismatch between using the _() macro which
is for translations, and using the errdetail_internal which is for
strings *not* requiring translation?

~~~

InvalidatePossiblyObsoleteSlot:

4.
/*
 * The logical replication slots shouldn't be invalidated as GUC
 * max_slot_wal_keep_size is set to -1 during the binary upgrade. See
 * check_old_cluster_for_valid_slots() where we ensure that no
 * invalidated before the upgrade.
 */
Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));

Unless I am mistaken, all of the v63 cleanups of the above binary
upgrade code assert stuff have vanished somewhere between v63 and v64.
I cannot find them in the spin-off thread. All accidentally lost? (in
2 places)

Not only that but the accompanying comment modification (to mention
"and idle_replication_slot_timeout is set to 0") is also MIA last seen
in v63 (??)

======

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: vignesh C <[email-masked]>
Date: Wed, 29 Jan 2025 12:44:26 +0530

>   2) 002 patch: Separates the TAP test "044_invalidate_inactive_slots"

Currently the test takes around 220 seconds for me. We could do the
following changes to bring it down to around 70 to 80 seconds:
1) Set idle_replication_slot_timeout to 70 seconds
+# Avoid unpredictability
+$primary->append_conf(
+       'postgresql.conf', qq{
+checkpoint_timeout = 1h
+});
+$primary->start;

2) I felt just 1 second more is enough unless you anticipate a random
failure, the test passes for me:
+# Give enough time for inactive_since to exceed the timeout
+sleep($idle_timeout_1min * 60 + 10);

3) Since we will be setting it to 70 seconds above, changing the
configuration and reload is not required:
+# Set timeout GUC so that the next checkpoint will invalidate inactive slots
+$primary->safe_psql(
+       'postgres', qq[
+    ALTER SYSTEM SET idle_replication_slot_timeout TO
'${idle_timeout_1min}min';
+]);
+$primary->reload;

4) Here you can add some comments that 60s has elapsed and the slot
will get invalidated in another 10 seconds, and pass timeout as 10s to
wait_for_slot_invalidation:
+# Wait for logical failover slot to become inactive on the primary. Note that
+# nobody has acquired the slot yet, so it must get invalidated due to
+# idle timeout.
+wait_for_slot_invalidation($primary, 'sync_slot1', $logstart,
+       $idle_timeout_1min);

5) We can have another streaming replication cluster setup, may be
primary2 and standby2 nodes and stop the standby2 immediately along
with the first streaming replication cluster itself:
+# Make the standby slot on the primary inactive and check for invalidation
+$standby1->stop;
+wait_for_slot_invalidation($primary, 'sb_slot1', $logstart,
+       $idle_timeout_1min);

6) We can rename primary to primary or standby1 to standby to keep the
name consistent:
+# Create standby slot on the primary
+$primary->safe_psql(
+       'postgres', qq[
+    SELECT pg_create_physical_replication_slot(slot_name :=
'sb_slot1', immediately_reserve := true);
+]);
+
+# Create standby
+my $standby1 = PostgreSQL::Test::Cluster->new('standby1');
+$standby1->init_from_backup($primary, $backup_name, has_streaming => 1);

Regards,
Vignesh

---

From: Amit Kapila <[email-masked]>
Date: Wed, 29 Jan 2025 14:07:07 +0530

> following changes to bring it down to around 70 to 80 seconds:

Even then it is too long for a single test to be part of committed
code. So, we can temporarily reduce its time but fixing comments on
this is not a good use of time. We need to write this test in some
other way if we want to see it committed.

-- 
With Regards,
Amit Kapila.

---

From: Nisha Moond <[email-masked]>
Date: Fri, 31 Jan 2025 07:43:16 +0530

> [1] https://www.postgresql.org/message-id/CABdArM6pBL5hPnSQ%2B5nEVMANcF4FCH7LQmgskXyiLY75TMnKpw%40mail.gmail.com

Please find the v65 patch set attached with following changes:

 - patch-0001 is the copy of v4-001 patch used as a base patch from
[1], as suggested by Peter in [2].
 - patch-0002 is the main patch implementing the feature, this has
also addressed the comments from [2] and [3]
 - patch-0003 adds an alternative approach for the TAP test using
injection points to force idle_timeout slot invalidation without
waiting for a minute. This test takes 2-3 seconds to complete.
 - patch-0004 maintains the previous test(v64-0002) which is under
PG_TEST_EXTRA. Also, addressed Vignesh's comments [4] for the test and
now it takes 70-80 seconds to complete.

Note: Patches 0003 and 0004 contain the same TAP test but use
different verification methods. We need to decide which one to keep.

[1] https://www.postgresql.org/message-id/CALDaNm1ZUHnKm%2BPSjjqRrMxcLagrUTS6SADnEsQBfW8rMZFrDA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPtyUQGee6pHkNN3-ghYhWnY5p-3yWumK7zKupu0S1oVQQ%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CALDaNm1J_mdqCYjQZgfQMVhJrxndPem5ruxpG_67t4C_2My9WQ%40mail.gmail.com
[4] https://www.postgresql.org/message-id/CALDaNm2dAJB%3DfJ2X7EMb7meNTjMyL-%2B-xA93JL_jPkGF4%3DRUYw%40mail.gmail.com

Thank you, Kuroda-san for providing the TAP test using injection
points (patch-0003).

--
Thanks,
Nisha

---

From: Peter Smith <[email-masked]>
Date: Fri, 31 Jan 2025 16:09:46 +1100

Hi Nisha.

Here are some review comments for patch v65-0002

======

ReportSlotInvalidation:

1.
+

errdetail:

I guess it is no fault of this patch because I see you've only copied
nearby code, but AFAICT this function is still having an each-way bet
by using a mixture of _() macro which is for strings intended be
translated, but then only using them in errdetail_internal() which is
for strings that are NOT intended to be translated. Isn't it
contradictory? Why don't we use errdetail() here?

errhint:

Also, the way the 'hint' is implemented can only be meaningful for
RS_INVAL_WAL_REMOVED. This is also existing code that IMO it was
always strange, but now that this patch has added another kind of
switch (cause) this hint implementation now looks increasingly hacky
to me; it is also inflexible -- e.g. if you ever wanted to add
different hints. A neater implementation would be to make the code
more like how the err_detail is handled, so then the errhint string
would only be assigned within the "case RS_INVAL_WAL_REMOVED:"

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

---

From: Amit Kapila <[email-masked]>
Date: Fri, 31 Jan 2025 14:32:34 +0530

> contradictory? Why don't we use errdetail() here?

Your question is valid and I don't have an answer. I encourage you to
start a new thread to clarify this.

> would only be assigned within the "case RS_INVAL_WAL_REMOVED:"

This makes sense to me.

+

I think the above message should be constructed on a model similar to
the following nearby message:"The slot's restart_lsn %X/%X exceeds the
limit by %llu bytes.". So, how about the following: "The slot's idle
time %s exceeds the configured \"%s\" duration"?

Also, similar to max_slot_wal_keep_size, we should give a hint in this
case to increase idle_replication_slot_timeout.

It is not clear why the injection point test is doing
pg_sync_replication_slots() etc. in the patch. The test should be
simple such that after creating a new physical or logical slot, enable
the injection point, then run the manual checkpoint command, and check
the invalidation status of the slot.

-- 
With Regards,
Amit Kapila.

---

From: Nisha Moond <[email-masked]>
Date: Fri, 31 Jan 2025 17:50:26 +0530

> the invalidation status of the slot.

Thanks for the review! I have incorporated the above comments. The
test in patch-002 has been optimized as suggested and now completes in
less than a second.
Please find the attached v66 patch set. The base patch(v65-001) is
committed now, so I have rebased the patches.

Thank you, Kuroda-san, for working on patch-002.

--
Thanks,
Nisha

---

From: vignesh C <[email-masked]>
Date: Sat, 1 Feb 2025 11:41:53 +0530

> committed now, so I have rebased the patches.

Few comments:
1)We should set inactive_since only if the slot can be invalidated:
+                                       /* For testing timeout slot
invalidation */
+                                       if
(IS_INJECTION_POINT_ATTACHED("slot-time-out-inval"))
+                                               s->inactive_since = 1;
+

2) Instead of "alter system set" and reload, let's do this in
$node->append_conf itself:
+# Set timeout GUC so that the next checkpoint will invalidate inactive slots
+$node->safe_psql(
+       'postgres', qq[
+    ALTER SYSTEM SET idle_replication_slot_timeout TO '1min';
+]);
+$node->reload;

3) No need to trigger checkpoint twice, we can move it outside so that
just a single checkpoint will invalidate both the slots:
+sub trigger_slot_invalidation
+{
+       my ($node, $slot, $offset) = @_;
+       my $node_name = $node->name;
+       my $invalidated = 0;
+
+       # Run a checkpoint
+       $node->safe_psql('postgres', "CHECKPOINT");

4) I fel this trigger_slot_invalidation is not required after removing
the checkpoint from the function, let's move the waiting for
"invalidating obsolete replication slot" also to
wait_for_slot_invalidation function:
+       # The slot's invalidation should be logged
+       $node->wait_for_log(qr/invalidating obsolete replication slot
\"$slot\"/,
+               $offset);
+
+       # Check that the invalidation reason is 'idle_timeout'
+       $node->poll_query_until(
+               'postgres', qq[
+               SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+                       WHERE slot_name = '$slot' AND
+                       invalidation_reason = 'idle_timeout';
+       ])

5) Can we move the subroutine to the beginning, I noticed in other
places we have kept it before the tests like in 027_nosuperuser and
040_createsubscriber:
+# Wait for slot to first become idle and then get invalidated
+sub wait_for_slot_invalidation
+{
+       my ($node, $slot, $offset) = @_;
+       my $node_name = $node->name;
+
+       trigger_slot_invalidation($node, $slot, $offset);
+
+       # Check that an invalidated slot cannot be acquired
+       my ($result, $stdout, $stderr);
+       ($result, $stdout, $stderr) = $node->psql(
+               'postgres', qq[
+                       SELECT pg_replication_slot_advance('$slot', '0/1');
+       ]);

6) Since idle_replication_slot_timeout is related more closely with
max_slot_wal_keep_size, let's keep it along with it.
diff --git a/src/backend/utils/misc/postgresql.conf.sample
b/src/backend/utils/misc/postgresql.conf.sample
index 079efa1baa..0ed9eb057e 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -329,6 +329,7 @@
 #wal_sender_timeout = 60s      # in milliseconds; 0 disables
 #track_commit_timestamp = off  # collect timestamp of transaction commit
                                # (change requires restart)
+#idle_replication_slot_timeout = 1d    # in minutes; 0 disables

If you accept the comments, you can merge the changes from the attached patch.

Regards,
Vignesh
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c09b90eeac..e81bce348c 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1724,20 +1724,22 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
 				case RS_INVAL_IDLE_TIMEOUT:
 					Assert(now > 0);
 
-					/* For testing timeout slot invalidation */
-					if (IS_INJECTION_POINT_ATTACHED("slot-time-out-inval"))
-						s->inactive_since = 1;
-
-					/*
-					 * Check if the slot needs to be invalidated due to
-					 * idle_replication_slot_timeout GUC.
-					 */
-					if (CanInvalidateIdleSlot(s) &&
-						TimestampDifferenceExceedsSeconds(s->inactive_since, now,
-														  idle_replication_slot_timeout_mins * SECS_PER_MINUTE))
+					if (CanInvalidateIdleSlot(s))
 					{
-						invalidation_cause = cause;
-						inactive_since = s->inactive_since;
+						/* For testing timeout slot invalidation */
+						if (IS_INJECTION_POINT_ATTACHED("slot-time-out-inval"))
+							s->inactive_since = 1;
+
+						/*
+						 * 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))
+						{
+							invalidation_cause = cause;
+							inactive_since = s->inactive_since;
+						}
 					}
 					break;
 				case RS_INVAL_NONE:
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 0ed9eb057e..70be3a2ce5 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -326,10 +326,10 @@
 				# (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 = 1d	# in minutes; 0 disables
 #wal_sender_timeout = 60s	# in milliseconds; 0 disables
 #track_commit_timestamp = off	# collect timestamp of transaction commit
 				# (change requires restart)
-#idle_replication_slot_timeout = 1d	# in minutes; 0 disables
 
 # - Primary Server -
 
diff --git a/src/test/recovery/t/044_invalidate_inactive_slots.pl b/src/test/recovery/t/044_invalidate_inactive_slots.pl
index fd907e7f82..7f18881cba 100644
--- a/src/test/recovery/t/044_invalidate_inactive_slots.pl
+++ b/src/test/recovery/t/044_invalidate_inactive_slots.pl
@@ -13,6 +13,39 @@ 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
+{
+	my ($node, $slot, $offset) = @_;
+	my $node_name = $node->name;
+
+	# The slot's invalidation should be logged
+	$node->wait_for_log(qr/invalidating obsolete replication slot \"$slot\"/,
+		$offset);
+
+	# Check that the invalidation reason is 'idle_timeout'
+	$node->poll_query_until(
+		'postgres', qq[
+		SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+			WHERE slot_name = '$slot' AND
+			invalidation_reason = 'idle_timeout';
+	])
+	  or die
+	  "Timed out while waiting for invalidation reason of slot $slot to be set on node $node_name";
+
+	# Check that an invalidated slot cannot be acquired
+	my ($result, $stdout, $stderr);
+	($result, $stdout, $stderr) = $node->psql(
+		'postgres', qq[
+			SELECT pg_replication_slot_advance('$slot', '0/1');
+	]);
+	ok( $stderr =~ /can no longer access replication slot "$slot"/,
+		"detected error upon trying to acquire invalidated slot $slot on node $node_name"
+	  )
+	  or die
+	  "could not detect error upon trying to acquire invalidated slot $slot on node $node_name";
+}
+
 # ========================================================================
 # Testcase start
 #
@@ -27,6 +60,7 @@ $node->init(allows_streaming => 'logical');
 $node->append_conf(
 	'postgresql.conf', qq{
 checkpoint_timeout = 1h
+idle_replication_slot_timeout = 1min
 });
 $node->start;
 
@@ -41,13 +75,6 @@ $node->psql('postgres',
 
 my $logstart = -s $node->logfile;
 
-# Set timeout GUC so that the next checkpoint will invalidate inactive slots
-$node->safe_psql(
-	'postgres', qq[
-    ALTER SYSTEM SET idle_replication_slot_timeout TO '1min';
-]);
-$node->reload;
-
 # Register an injection point on the primary to forcibly cause a slot timeout
 $node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
 
@@ -62,6 +89,9 @@ if (!$node->check_extension('injection_points'))
 $node->safe_psql('postgres',
 	"SELECT injection_points_attach('slot-time-out-inval', 'error');");
 
+# Run a checkpoint which will invalidate the slots
+$node->safe_psql('postgres', "CHECKPOINT");
+	
 # Wait for slots to become inactive. Note that nobody has acquired the slot
 # yet, so it must get invalidated due to idle timeout.
 wait_for_slot_invalidation($node, 'physical_slot', $logstart);
@@ -70,50 +100,4 @@ wait_for_slot_invalidation($node, 'logical_slot', $logstart);
 # Testcase end
 # =============================================================================
 
-# Wait for slot to first become idle and then get invalidated
-sub wait_for_slot_invalidation
-{
-	my ($node, $slot, $offset) = @_;
-	my $node_name = $node->name;
-
-	trigger_slot_invalidation($node, $slot, $offset);
-
-	# Check that an invalidated slot cannot be acquired
-	my ($result, $stdout, $stderr);
-	($result, $stdout, $stderr) = $node->psql(
-		'postgres', qq[
-			SELECT pg_replication_slot_advance('$slot', '0/1');
-	]);
-	ok( $stderr =~ /can no longer access replication slot "$slot"/,
-		"detected error upon trying to acquire invalidated slot $slot on node $node_name"
-	  )
-	  or die
-	  "could not detect error upon trying to acquire invalidated slot $slot on node $node_name";
-}
-
-# Trigger slot invalidation and confirm it in the server log
-sub trigger_slot_invalidation
-{
-	my ($node, $slot, $offset) = @_;
-	my $node_name = $node->name;
-	my $invalidated = 0;
-
-	# Run a checkpoint
-	$node->safe_psql('postgres', "CHECKPOINT");
-
-	# The slot's invalidation should be logged
-	$node->wait_for_log(qr/invalidating obsolete replication slot \"$slot\"/,
-		$offset);
-
-	# Check that the invalidation reason is 'idle_timeout'
-	$node->poll_query_until(
-		'postgres', qq[
-		SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
-			WHERE slot_name = '$slot' AND
-			invalidation_reason = 'idle_timeout';
-	])
-	  or die
-	  "Timed out while waiting for invalidation reason of slot $slot to be set on node $node_name";
-}
-
 done_testing();

---

From: Peter Smith <[email-masked]>
Date: Mon, 3 Feb 2025 11:46:17 +1100

Hi Nisha,

Some review comments for v66-0001.

======

ReportSlotInvalidation:

1.
  StringInfoData err_detail;
+ StringInfoData err_hint;
  bool hint = false;

  initStringInfo(&err_detail);
+ initStringInfo(&err_hint);

I don't think you still need the 'hint' boolean anymore.

Instead of:
hint ? errhint("%s", err_hint.data) : 0);

You could just do something like:
err_hint.len ? errhint("%s", err_hint.data) : 0);

~~~

2.
+ appendStringInfo(&err_hint, "You might need to increase \"%s\".",
+ "max_slot_wal_keep_size");
  break;
2a.
In this case, shouldn't you really be using macro _("You might need to
increase \"%s\".") so that the common format string would be got using
gettext()?

~

2b.
Should you include a /* translator */ comment here? Other places where
GUC name is substituted do this.

~~~

3.
+ appendStringInfo(&err_hint, "You might need to increase \"%s\".",
+ "idle_replication_slot_timeout");
+ break;

3a.
Ditto above. IMO this common format string should be got using macro.
e.g.: _("You might need to increase \"%s\".")

~

3b.
Should you include a /* translator */ comment here? Other places where
GUC name is substituted do this.

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

---

From: Peter Smith <[email-masked]>
Date: Mon, 3 Feb 2025 14:33:47 +1100

> start a new thread to clarify this.

I think this was a false alarm.

After studying this more deeply, I've changed my mind and now think
the code is OK as-is.

AFAICT errdetail_internal is used when not wanting to translate the
*fmt* string passed to it (see EVALUATE_MESSAGE in elog.c). Now, here
the format string is just "%s" so it's fine to not translate that.
Meanwhile, the string value being substituted to the "%s" was already
translated because of the _(x) macro aka gettext(x).

I found other examples similar to this -- see the
error_view_not_updatable() function in rewriteHandler.c which does:
ereport(ERROR,
...
 detail ? errdetail_internal("%s", _(detail)) : 0,
...

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

---

From: Amit Kapila <[email-masked]>
Date: Mon, 3 Feb 2025 10:34:35 +0530

> translated because of the _(x) macro aka gettext(x).

I didn't get your point about " the "%s" was already translated
because of ...". If we don't want to translate the message then why
add '_(' to it in the first place?

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Mon, 3 Feb 2025 16:50:56 +1100

> add '_(' to it in the first place?

I think this is same point where I was fooling myself yesterday.  In
fact we do want to translate the message seen by the user.

errdetail_internal really means don't translate the ***format
string***. In our case "%s" is not the message at all -- it is just
the a *format string* so translating "%s" is kind of meaningless.

e.g. Normally....

errdetail("translate me") <-- This would translate the fmt string but
here the fmt is also the message; i.e. it will do gettext("translate
me") internally.

errdetail_internal("translate me") <-- This won't translate anything;
you will have the raw fmt string "translate me"

~~

But since ReportSlotInvalidation is building the message on the fly
there is no single report so it is a bit different....

errdetail("%s", "translate me") <-- this would just use gettext("%s")
which is kind of useless. And the "translate me" is just a raw string
and won't be translated.

errdetail_internal("%s", "translate me") <-- this won't translate
anything; the fmt string and the "translate me" are just raw strings

errdetail_internal("%s", _("translate me"))  <-- This won't translate
the fmt string, but to translate %s is useless anyway. OTOH, the _()
macro means it will do gettext("translate me") so the "translate me"
string will get translated before it is substituted. This is
effectively what the ReportSlotInvalidation code is doing.

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

---

From: Amit Kapila <[email-masked]>
Date: Mon, 3 Feb 2025 12:04:36 +0530

> e.g.: _("You might need to increase \"%s\".")

Instead, we can directly use '_(' in errhint as we are doing in one
other similar place "errhint("%s", _(view_updatable_error))));". I
think we didn't use it for errdetail because, in one of the cases, it
needs to use ngettext

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 3 Feb 2025 14:55:05 +0530

> committed now, so I have rebased the patches.

*
       <para>
         The time when the slot became inactive. <literal>NULL</literal> if the
-        slot is currently being streamed.
+        slot is currently being streamed. If the slot becomes invalidated,
+        this value will remain unchanged until server shutdown.
...
@@ -2408,7 +2527,9 @@ RestoreSlotFromDisk(const char *name)
  /*
  * Set the time since the slot has become inactive after loading the
  * slot from the disk into memory. Whoever acquires the slot i.e.
- * makes the slot active will reset it.
+ * makes the slot active will reset it. Avoid calling
+ * ReplicationSlotSetInactiveSince() here, as it will not set the time
+ * for invalid slots.
  */
  slot->inactive_since = GetCurrentTimestamp();

It looks inconsistent to set inactive_since on restart for invalid
slots but not at other times. We don't need to set inactive_since for
invalid slots. The invalid slots should not be updated. Ideally, this
should be taken care in the patch that introduces inactive_since but
we can do that now. Let's do this as a separate patch altogether in a
new thread.

-- 
With Regards,
Amit Kapila.

---

From: Nisha Moond <[email-masked]>
Date: Mon, 3 Feb 2025 17:35:22 +0530

> we can do that now. Let's do this as a separate patch altogether in a

Created a new thread [1] to address the inactive_since update for
invalid slots in a separate patch.

[1] https://www.postgresql.org/message-id/CABdArM7QdifQ_MHmMA%3DCc4v8%2BMeckkwKncm2Nn6tX9wSCQ-%2Biw%40mail.gmail.com

--
Thanks,
Nisha

---

From: Shlok Kyal <[email-masked]>
Date: Mon, 3 Feb 2025 18:35:43 +0530

Hi Nisha,

I reviewed the v66 patch. I have few comments:

1. I also feel the default value should be set to '0' as suggested by
Vignesh in 1st point of [1].

2. Should we allow copying of invalidated slots?
Currently we are able to copy slots which are invalidated:

postgres=# select slot_name, active, restart_lsn, wal_status,
inactive_since , invalidation_reason from pg_replication_slots;
 slot_name | active | restart_lsn | wal_status |
inactive_since          | invalidation_reason
-----------+--------+-------------+------------+----------------------------------+---------------------
 test1     | f      | 0/16FDDE0   | lost       | 2025-02-03
18:28:01.802463+05:30 | idle_timeout
(1 row)

postgres=# select pg_copy_logical_replication_slot('test1', 'test2');
 pg_copy_logical_replication_slot
----------------------------------
 (test2,0/16FDE18)
(1 row)

postgres=# select slot_name, active, restart_lsn, wal_status,
inactive_since , invalidation_reason from pg_replication_slots;
 slot_name | active | restart_lsn | wal_status |
inactive_since          | invalidation_reason
-----------+--------+-------------+------------+----------------------------------+---------------------
 test1     | f      | 0/16FDDE0   | lost       | 2025-02-03
18:28:01.802463+05:30 | idle_timeout
 test2     | f      | 0/16FDDE0   | reserved   | 2025-02-03
18:29:53.478023+05:30 |
(2 rows)

3. We have similar behaviour as above for physical slots.

[1]: https://www.postgresql.org/message-id/CALDaNm14QrW5j6su%2BEAqjwnHbiwXJwO%2Byk73_%3D7yvc5TVY-43g%40mail.gmail.com

Thanks and Regards,
Shlok Kyal

---

From: Peter Smith <[email-masked]>
Date: Tue, 4 Feb 2025 09:32:17 +1100

> think we didn't use it for errdetail because, in one of the cases, it

-1 for this suggestion because this will end up causing a gettext() on
the entire hint where the GUC has already been substituted.

e.g. it is effectively doing

_("You might need to increase \"max_slot_wal_keep_size\".")
_("You might need to increase \"idle_replication_slot_timeout\".")

But that is contrary to the goal of reducing the burden on translators
by using *common* messages wherever possible. IMO we should only
request translation of the *common* part of the hint message.

e.g.
_("You might need to increase \"%s\".")

~~~

We always do GUC name substitution into a *common* fmt message because
then translators only need to maintain a single translated string
instead of many. You can find examples of this everywhere. For
example, notice the GUC is always substituted into the translated fmt
msgs below; they never have the GUC name included explicitly. The
result is just a single fmt message is needed.

$ grep -r . -e 'errhint("You might need to increase' | grep '.c:'
./src/backend/replication/logical/launcher.c: errhint("You might need
to increase \"%s\".", "max_logical_replication_workers")));
./src/backend/replication/logical/launcher.c: errhint("You might need
to increase \"%s\".", "max_worker_processes")));
./src/backend/storage/lmgr/predicate.c: errhint("You might need to
increase \"%s\".", "max_pred_locks_per_transaction")));
./src/backend/storage/lmgr/predicate.c: errhint("You might need to
increase \"%s\".", "max_pred_locks_per_transaction")));
./src/backend/storage/lmgr/predicate.c: errhint("You might need to
increase \"%s\".", "max_pred_locks_per_transaction")));
./src/backend/storage/lmgr/lock.c: errhint("You might need to increase
\"%s\".", "max_locks_per_transaction")));
./src/backend/storage/lmgr/lock.c: errhint("You might need to increase
\"%s\".", "max_locks_per_transaction")));
./src/backend/storage/lmgr/lock.c: errhint("You might need to increase
\"%s\".", "max_locks_per_transaction")));
./src/backend/storage/lmgr/lock.c: errhint("You might need to increase
\"%s\".", "max_locks_per_transaction")));
./src/backend/storage/lmgr/lock.c: errhint("You might need to increase
\"%s\".", "max_locks_per_transaction")));

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 4 Feb 2025 10:38:39 +0530

> request translation of the *common* part of the hint message.

Fair point. So, we can ignore my suggestion.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 4 Feb 2025 10:45:09 +0530

> Vignesh in 1st point of [1].

+1. This will ensure that the idle slots won't be invalidated by
default, the same as HEAD. We can change the default value based on
user inputs.

> inactive_since          | invalidation_reason

Is this related to this patch or the behavior of HEAD? If this
behavior is not introduced by this patch then we should discuss this
in a separate thread. I couldn't think of why anyone wants to copy the
invalid slots, so we should probably prohibit copying invalid slots
but that is a matter of separate discussion unless introduced by this
patch.

-- 
With Regards,
Amit Kapila.

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 4 Feb 2025 15:28:05 +0530

> but that is a matter of separate discussion unless introduced by this

Hi Amit,

I tested and found that this issue is present in HEAD as well.

There are three types of invalidation in HEAD:
1. "wal_removed"
2. "rows_removed"
3. "wal_level_insufficient"

for copying slot with invalidation "wal_removed" we get an error:

postgres=# select slot_name, active, active_pid, restart_lsn,
wal_status, invalidation_reason from pg_replication_slots;
slot_name | active | active_pid | restart_lsn | wal_status | invalidation_reason
-----------+--------+------------+-------------+------------+---------------------
test1     | f      |            |             | lost       | wal_removed
(1 row)
postgres=#  select pg_copy_logical_replication_slot('test1', 'test2');
ERROR:  cannot copy a replication slot that doesn't reserve WAL

But for slot with invalidation "rows_removed" and
"wal_level_insufficient" we are able to copy the slot:

postgres=# select slot_name, active, active_pid, restart_lsn,
wal_status, invalidation_reason from pg_replication_slots;
slot_name | active | active_pid | restart_lsn | wal_status | invalidation_reason
-----------+--------+------------+-------------+------------+---------------------
slot1     | f      |            | 0/302E718   | lost       | rows_removed
(1 row)
postgres=# select pg_copy_logical_replication_slot('slot1', 'slot2');
pg_copy_logical_replication_slot
----------------------------------
(slot2,0/302E770)
(1 row)
postgres=# select slot_name, active, active_pid, restart_lsn,
wal_status, invalidation_reason from pg_replication_slots;
slot_name | active | active_pid | restart_lsn | wal_status | invalidation_reason
-----------+--------+------------+-------------+------------+---------------------
slot1     | f      |            | 0/302E718   | lost       | rows_removed
slot2     | f      |            | 0/302E718   | reserved   |
(2 rows)

Similarly we can copy slot with invalidation "wal_level_insufficient".
I have started a new thread to address the issue [1].

[1]: https://www.postgresql.org/message-id/CANhcyEU65aH0VYnLiu=[email-masked]

Thanks and Regards,
Shlok Kyal

---

From: Nisha Moond <[email-masked]>
Date: Tue, 4 Feb 2025 15:57:51 +0530

> default, the same as HEAD. We can change the default value based on

Here are the v68 patches, incorporating above as well as comments from [1].

Note: The 0003 patch with tests under PG_EXTRA_TESTS is not included
for now. If needed, I'll send it later once the first two patches are
committed.

[1] https://www.postgresql.org/message-id/CAHut%2BPv3mjQxmv5tHfgX%3Do%3D4C2TfX5rNYGS7xWrHBGcSVwr3mQ%40mail.gmail.com

--
Thanks,
Nisha

---

From: vignesh C <[email-masked]>
Date: Tue, 4 Feb 2025 16:41:47 +0530

> Here are the v68 patches, incorporating above as well as comments from [1].
Few comments:
1) Let's call TimestampDifferenceExceedsSeconds only if
idle_replication_slot_timeout_mins is set to avoid the
TimestampDifferenceExceedsSeconds function call and timestamp diff
calculation if not required:
+ if (CanInvalidateIdleSlot(s) &&
+ TimestampDifferenceExceedsSeconds(s->inactive_since, now,
+   idle_replication_slot_timeout_mins * SECS_PER_MINUTE))
+ {
+ invalidation_cause = cause;
+ inactive_since = s->inactive_since;
+ }
+ break;

2) Let's keep the prototype after TimestampDifferenceExceeds to keep
it consistent with the source file and will also make it easy to
search:
diff --git a/src/include/utils/timestamp.h b/src/include/utils/timestamp.h
index d26f023fb8..e1d05d6779 100644
--- a/src/include/utils/timestamp.h
+++ b/src/include/utils/timestamp.h
@@ -143,5 +143,8 @@ extern int  date2isoyear(int year, int mon, int mday);
 extern int     date2isoyearday(int year, int mon, int mday);

 extern bool TimestampTimestampTzRequiresRewrite(void);
+extern bool TimestampDifferenceExceedsSeconds(TimestampTz start_time,
+
                   TimestampTz stop_time,
+
                   int threshold_sec);

3)How about we change the below:
+#ifdef USE_INJECTION_POINTS
+
+                                               /*
+                                                * To test idle
timeout slot invalidation, if the
+                                                * slot-time-out-inval
injection point is attached,
+                                                * set inactive_since
to a very old timestamp (1
+                                                * microsecond since
epoch) to immediately invalidate
+                                                * the slot.
+                                                */
+                                               if
(IS_INJECTION_POINT_ATTACHED("slot-time-out-inval"))
+                                                       s->inactive_since = 1;
+#endif
to:
#ifdef USE_INJECTION_POINTS
/*
* To test idle timeout slot invalidation, if the
* slot-time-out-inval injection point is attached,
* set inactive_since to current time and invalidate the slot immediately.
*/
if (IS_INJECTION_POINT_ATTACHED("slot-time-out-inval") &&
idle_replication_slot_timeout_mins)
{
invalidation_cause = cause;
inactive_since = s->inactive_since = now;
}
#else
/*
* 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))
{
invalidation_cause = cause;
inactive_since = s->inactive_since;
}
#endif

We can just invalidate the slot directly without checking the time
difference if idle_replication_slot_timeout_mins is set and
inactive_since can hold the now value.

Regards,
Vignesh

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 4 Feb 2025 12:16:28 +0000

Dear Nisha,

Thanks for updating the patch! Here are my comments.

01.
```
+# Test for replication slots invalidation
```

Since the file tests only timeout invalidations, the comment seems too general.

02.
```
```

This part can be removal because this is not directly related with timeout invalidation.
If needed this can be outside the function and we can confirm only once.

03.
```
+# Initialize primary
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init(allows_streaming => 'logical');
```

I think this node is not "primary" because there are no standby nodes. We can use new('node').
Also some comments which used "primary" can be removed.

04.
```
+$node->psql('postgres',
+       q{SELECT pg_create_logical_replication_slot('logical_slot', 'test_decoding');}
+);
```

Please use safe_psql() instead of psql().

05.
```
```

According to other tests, the variable name can be $log_offset.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Nisha Moond <[email-masked]>
Date: Tue, 4 Feb 2025 19:56:00 +0530

> + inactive_since = s->inactive_since;

The CanInvalidateIdleSlot(s) call does the check if
idle_replication_slot_timeout_mins is set or not. So we are good here.

> +extern bool TimestampDifferenceExceedsSeconds(TimestampTz start_time,

Done.

> inactive_since can hold the now value.

+1 to the idea. Implemented it in a slightly different way to avoid
enclosing the main code within "#else".

Here is v69 patch set addressing above and Kuroda-san's comments in [1].

[1] https://www.postgresql.org/message-id/OSCPR01MB14966A918EBB0674E5423EDE0F5F42%40OSCPR01MB14966.jpnprd01.prod.outlook.com

--
Thanks,
Nisha

---

From: Peter Smith <[email-masked]>
Date: Wed, 5 Feb 2025 14:13:34 +1100

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.

======

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

---

From: vignesh C <[email-masked]>
Date: Wed, 5 Feb 2025 10:29:52 +0530

> Here is v69 patch set addressing above and Kuroda-san's comments in [1].

Few minor suggestions:
1) In the slot invalidation reporting below:
+               case RS_INVAL_IDLE_TIMEOUT:
+                       Assert(inactive_since > 0);
+
+                       /* translator: second %s is a GUC variable name */
+                       appendStringInfo(&err_detail, _("The slot's
idle time %s exceeds the configured \"%s\" duration."),
+
timestamptz_to_str(inactive_since),
+
"idle_replication_slot_timeout");
+                       /* translator: %s is a GUC variable name */
+                       appendStringInfo(&err_hint, _("You might need
to increase \"%s\"."),
+
"idle_replication_slot_timeout");

It is logged like:
2025-02-05 10:04:11.616 IST [330567] DETAIL:  The slot's idle time
2025-02-05 10:02:49.131631+05:30 exceeds the configured
"idle_replication_slot_timeout" duration.

Here even though we tell idle time, we are logging the inactive_since
value which kind of gives a wrong meaning.

How about we change it to:
The slot has been inactive since 2025-02-05 10:02:49.131631+05:30,
which exceeds the configured "idle_replication_slot_timeout" duration.

2) Here we have mentioned about invalidation happens only for a)
released slots b) inactive slots replication slots c) slot where
communication between pub and sub is down
+                * XXX: Slot invalidation due to 'idle_timeout' applies only to
+                * released slots, and is based on the
'idle_replication_slot_timeout'
+                * GUC. Active slots currently in use for replication
are excluded to
+                * prevent accidental invalidation. Slots where
communication between
+                * the publisher and subscriber is down are also
excluded, as they are
+                * managed by the 'wal_sender_timeout'.
+                */
+               InvalidateObsoleteReplicationSlots(RS_INVAL_IDLE_TIMEOUT,
+
            0,
+
            InvalidOid,
+
            InvalidTransactionId);
a) Can we include about slots which does not reserve WAL are also not
considered.
b) Could we present this in a bullet-point format like the following:
+                * XXX: Slot invalidation due to 'idle_timeout' applies only to:
+                * 1) released slots, and is based on the
'idle_replication_slot_timeout'
+                * GUC. 2) Active slots currently in use for
replication are excluded to
+                * prevent accidental invalidation. 3) Slots where
communication between
+                * the publisher and subscriber is down are also
excluded, as they are
+                * managed by the 'wal_sender_timeout'.
+                */
c) While I was initially reviewing the patch I also had the similar
thoughts on my mind, if we could mention the one like "Slots where
communication between the publisher and subscriber is down are also
excluded, as they are managed by the 'wal_sender_timeout'" in the
documentation it might be good.

Regards,
Vignesh

---

From: Peter Smith <[email-masked]>
Date: Wed, 5 Feb 2025 18:28:09 +1100

Hi Nisha,

Some review comments for the patch v69-0002.

======

1.
+#ifdef USE_INJECTION_POINTS
+
+ /*
+ * To test idle timeout slot invalidation, if the
+ * slot-time-out-inval injection point is attached,
+ * immediately invalidate the slot.
+ */
+ if (IS_INJECTION_POINT_ATTACHED("slot-time-out-inval"))
+ {
+ invalidation_cause = cause;
+ inactive_since = s->inactive_since = now;
+ break;
+ }
+#endif

1a.
I didn't understand the reason for the assignment ' = now' here. This
is not happening in the normal code path so why do you need to do this
in this test code path? It works for me without doing this.

~

1b.
For testing, I think we should try to keep the injection code
differences minimal -- e.g. share the same (normal build) code as much
as possible. For example, I suggest refactoring like below. Well, it
works for me.

/*
 * Check if the slot needs to be invalidated due to
 * idle_replication_slot_timeout GUC.
 *
 * To test idle timeout slot invalidation, if the
 * "slot-time-out-inval" injection point is attached,
 * immediately invalidate the slot.
 */
if (
#ifdef USE_INJECTION_POINTS
  IS_INJECTION_POINT_ATTACHED("slot-time-out-inval") ||
#endif
  TimestampDifferenceExceedsSeconds(s->inactive_since, now,
    idle_replication_slot_timeout_mins * SECS_PER_MINUTE))
{
  invalidation_cause = cause;
  inactive_since = s->inactive_since;
}

~

1c.
Can we call the injection point "timeout" instead of "time-out"?

======
.../t/044_invalidate_inactive_slots.pl

2.
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}

At first, I had no idea how to build for this test. It would be good
to include a link to the injection build instructions in a comment
somewhere near here.

~~~

3.

Might be better to call the variable $slot_name instead of $slot.

Also then it will be consistent with $node_name

~~~

4.
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.

I misread this comment at first -- maybe it is clearer to reverse the wording?

/extension injection_points/’injection_points’ extension/

~~~

5.

The explanation seems a bit terse -- I think the comment should
elaborate a bit more to explain that CHECKPOINT is just where the idle
slot timeout is checked, but since the test is using injection point
and the injection code enforces immediate idle timeout THAT is why it
will invalidate the slots...

~~~

6.
+# Wait for slots to become inactive. Note that nobody has acquired the slot
+# yet, so it must get invalidated due to idle timeout.

IIUC this comment means:

SUGGESTION
Note that since nobody has acquired the slot yet, then if it has been
invalidated that can only be due to the idle timeout mechanism.

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

---

From: Amit Kapila <[email-masked]>
Date: Wed, 5 Feb 2025 14:42:31 +0530

> which exceeds the configured "idle_replication_slot_timeout" duration.

Would it address your concern if we write the actual idle duration
(now - inactive_since) instead of directly using inactive_since in the
above message?

A few other comments:
1.
+ * 4. The slot is not being synced from the primary while the server
+ *    is in recovery
+ *
+ * Note that the idle timeout invalidation mechanism is not
+ * applicable for slots on the standby server that are being synced
+ * from the primary server (i.e., standby slots having 'synced' field 'true').
+ * Synced slots are always considered to be inactive because they don't
+ * perform logical decoding to produce changes.

The 4th point in the above comment and the rest of the comment is
mostly saying the same thing.

2.
+ * Flush all replication slots to disk. Also, invalidate obsolete slots during
+ * non-shutdown checkpoint.
  *
  * It is convenient to flush dirty replication slots at the time of checkpoint.
  * Additionally, in case of a shutdown checkpoint, we also identify the slots
@@ -1924,6 +2007,45 @@ CheckPointReplicationSlots(bool is_shutdown)

Can we try and see how the patch looks if we try to invalidate the
slot due to idle time at the same time when we are trying to
invalidate due to WAL?

-- 
With Regards,
Amit Kapila.

---

From: Nisha Moond <[email-masked]>
Date: Thu, 6 Feb 2025 08:02:07 +0530

> a) Can we include about slots which does not reserve WAL are also not

We have included all the info regarding which slots are excluded in
the documents, so I feel we can remove the XXX: comment from here.
(done in v70).

> documentation it might be good.

v70 adds the suggested info in the docs.

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Thu, 6 Feb 2025 08:02:13 +0530

> to include a link to the injection build instructions in a comment

I’ve added comments with build instructions in v70, but I’m not sure
if a link to the documentation is necessary. I didn’t find similar
instructions in other injection point-dependent tests. Let’s see what
others think.

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Thu, 6 Feb 2025 08:02:29 +0530

> (now - inactive_since) instead of directly using inactive_since in the

Simply using the raw timestamp difference (now - inactive_since) would
look odd. We should convert it into a user-friendly format. Since
idle_replication_slot_timeout is in minutes, we can express the
difference in minutes and seconds in the log.
For example:
DETAIL: The slot's idle time of 1 minute and 7 seconds exceeds the
configured "idle_replication_slot_timeout" duration.

This has been implemented in v70.
Thoughts?

> mostly saying the same thing.

Done. I've merged the additional info and 4th point.

> slot due to idle time at the same time when we are trying to

I'll consider the suggested change in the next version.
~~~~

Here are the v70 patches -  addressed above and other comments in [1],
[2] and [3].

[1] https://www.postgresql.org/message-id/CAHut%2BPvW3pr3P3hXwBskXrDmJYKedmqRaPZcL4iLRQ51%3DXxOBw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALDaNm0X_vgAxKPT%2Bc14yqKcgE5-x4XBdXsCAVqD6_aa-QYUvg%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CAHut%2BPtCpOnifF9wnhJ%3Djo7KLmtT%3DMikuYnM9GGPTVA80rq7OA%40mail.gmail.com

--
Thanks,
Nisha

---

From: Amit Kapila <[email-masked]>
Date: Thu, 6 Feb 2025 10:17:05 +0530

> configured "idle_replication_slot_timeout" duration.

This is better but the implementation should be done on the caller
side mainly because we don't want to call a new GetCurrentTimestamp()
in ReportSlotInvalidation.

> I'll consider the suggested change in the next version.

FYI, we discussed this previously (1), but the conclusion that it
won't help much (as it will not help to remove WAL immediately) is
incorrect, especially if we do what is suggested now.

Apart from this, I have made minor changes in the comments. Please
review and include them unless you disagree.

(1) - https://www.postgresql.org/message-id/CALj2ACXe8%2BxSNdMXTMaSRWUwX7v61Ad4iddUwnn%3DdjSwx3GLLg%40mail.gmail.com

-- 
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 29749ce917..4eb679e187 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1598,11 +1598,11 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause,
  * Idle timeout invalidation is allowed only when:
  *
  * 1. Idle timeout is set
- * 2. Slot has WAL reserved
+ * 2. Slot has reserved WAL
  * 3. Slot is inactive
- * 4. The slot is not being synced from the primary while the server
- *    is in recovery. As synced slots are always considered to be inactive
- *    because they don't perform logical decoding to produce changes.
+ * 4. The slot is not being synced from the primary while the server is in
+ *	  recovery. This is because synced slots are always considered to be
+ *	  inactive because they don't perform logical decoding to produce changes.
  */
 static inline bool
 CanInvalidateIdleSlot(ReplicationSlot *s)
@@ -1662,7 +1662,7 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
 		if (cause == RS_INVAL_IDLE_TIMEOUT)
 		{
 			/*
-			 * Assign the current time here to reduce system call overhead
+			 * Assign the current time here to avoid system call overhead
 			 * while holding the spinlock in subsequent code.
 			 */
 			now = GetCurrentTimestamp();

---

From: Amit Kapila <[email-masked]>
Date: Thu, 6 Feb 2025 10:19:21 +0530

> incorrect, especially if we do what is suggested now.

The above sentence is incomplete. Let me re-write it. We discussed
this previously, but the conclusion that it won't help much (as it
will not help to remove WAL immediately) at the time shutdown
checkpoint is incorrect, especially if we do what is suggested now.
So, we should try to invalidate the slots even during shutdown
checkpoints.

-- 
With Regards,
Amit Kapila.

---

From: Nisha Moond <[email-masked]>
Date: Thu, 6 Feb 2025 16:08:17 +0530

Done.

> > I'll consider the suggested change in the next version.

Done the changes as suggested in v71.

> review and include them unless you disagree.

Done.
~~~~
Here are the v71 patches with the above comments incorporated.

--
Thanks,
Nisha