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

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 3 Apr 2024 20:28:04 +0530

On Wed, Apr 3, 2024 at 6:46 PM Bertrand Drouvot
<[email-masked]> wrote:
>
> Just one comment on v32-0001:
>
> +# Synced slot on the standby must get its own inactive_since.
> +is( $standby1->safe_psql(
> +               'postgres',
> +               "SELECT '$inactive_since_on_primary'::timestamptz <= '$inactive_since_on_standby'::timestamptz AND
> +                       '$inactive_since_on_standby'::timestamptz <= '$slot_sync_time'::timestamptz;"
> +       ),
> +       "t",
> +       'synchronized slot has got its own inactive_since');
> +
>
> By using <= we are not testing that it must get its own inactive_since (as we
> allow them to be equal in the test). I think we should just add some usleep()
> where appropriate and deny equality during the tests on inactive_since.

Thanks. It looks like we can ignore the equality in all of the
inactive_since comparisons. IIUC, all the TAP tests do run with
primary and standbys on the single BF animals. And, it looks like
assigning the inactive_since timestamps to perl variables is giving
the microseconds precision level
(./tmp_check/log/regress_log_040_standby_failover_slots_sync:inactive_since
2024-04-03 14:30:09.691648+00). FWIW, we already have some TAP and SQL
tests relying on stats_reset timestamps without equality. So, I've
left the equality for the inactive_since tests.

> Except for the above, v32-0001 LGTM.

Thanks. Please see the attached v33-0001 patch after removing equality
on inactive_since TAP tests.

On Wed, Apr 3, 2024 at 1:47 PM Bertrand Drouvot
<[email-masked]> wrote:
>
> Some comments regarding v31-0002:
>
> === testing the behavior
>
> T1 ===
>
> > - synced slots don't get invalidated due to inactive timeout because
> > such slots not considered active at all as they don't perform logical
> > decoding (of course, they will perform in fast_forward mode to fix the
> > other data loss issue, but they don't generate changes for them to be
> > called as *active* slots)
>
> It behaves as described. OTOH non synced logical slots on the standby and
> physical slots on the standby are invalidated which is what is expected.

Right.

> T2 ===
>
> In case the slot is invalidated on the primary,
>
> primary:
>
> postgres=# select slot_name, inactive_since, invalidation_reason from pg_replication_slots where slot_name = 's1';
>  slot_name |        inactive_since         | invalidation_reason
> -----------+-------------------------------+---------------------
>  s1        | 2024-04-03 06:56:28.075637+00 | inactive_timeout
>
> then on the standby we get:
>
> standby:
>
> postgres=# select slot_name, inactive_since, invalidation_reason from pg_replication_slots where slot_name = 's1';
>  slot_name |        inactive_since        | invalidation_reason
> -----------+------------------------------+---------------------
>  s1        | 2024-04-03 07:06:43.37486+00 | inactive_timeout
>
> shouldn't the slot be dropped/recreated instead of updating inactive_since?

The sync slots that are invalidated on the primary aren't dropped and
recreated on the standby. There's no point in doing so because
invalidated slots on the primary can't be made useful. However, I
found that the synced slot is acquired and released unnecessarily
after the invalidation_reason is synced from the primary. I added a
skip check in synchronize_one_slot to skip acquiring and releasing the
slot if it's locally found inactive. With this, inactive_since won't
get updated for invalidated sync slots on the standby as we don't
acquire and release the slot.

> === code
>
> CR1 ===
>
> +        Invalidates replication slots that are inactive for longer the
> +        specified amount of time
>
> s/for longer the/for longer that/?

Fixed.

> CR2 ===
>
> +        <literal>true</literal>) as such synced slots don't actually perform
> +        logical decoding.
>
> We're switching in fast forward logical due to [1], so I'm not sure that's 100%
> accurate here. I'm not sure we need to specify a reason.

Fixed.

> CR3 ===
>
> + errdetail("This slot has been invalidated because it was inactive for more than the time specified by replication_slot_inactive_timeout parameter.")));
>
> I think we can remove "parameter" (see for example the error message in
> validate_remote_info()) and reduce it a bit, something like?
>
> "This slot has been invalidated because it was inactive for more than replication_slot_inactive_timeout"?

Done.

> CR4 ===
>
> + appendStringInfoString(&err_detail, _("The slot has been inactive for more than the time specified by replication_slot_inactive_timeout parameter."));
>
> Same.

Done. Changed it to "The slot has been inactive for more than
replication_slot_inactive_timeout."

> CR5 ===
>
> +       /*
> +        * This function isn't expected to be called for inactive timeout based
> +        * invalidation. A separate function InvalidateInactiveReplicationSlot is
> +        * to be used for that.
>
> Do you think it's worth to explain why?

Hm, I just wanted to point out the actual function here. I modified it
to something like the following, if others feel we don't need that, I
can remove it.

    /*
     * Use InvalidateInactiveReplicationSlot for inactive timeout based
     * invalidation.
     */

> CR6 ===
>
> +       if (replication_slot_inactive_timeout == 0)
> +               return false;
> +       else if (slot->inactive_since > 0)
>
> "else" is not needed here.

Nothing wrong there, but removed.

> CR7 ===
>
> +               SpinLockAcquire(&slot->mutex);
> +
> +               /*
> +                * Check if the slot needs to be invalidated due to
> +                * replication_slot_inactive_timeout GUC. We do this with the spinlock
> +                * held to avoid race conditions -- for example the inactive_since
> +                * could change, or the slot could be dropped.
> +                */
> +               now = GetCurrentTimestamp();
>
> We should not call GetCurrentTimestamp() while holding a spinlock.

I was thinking why to add up the wait time to acquire
LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);. Now that I
moved it up before the spinlock but after the LWLockAcquire.

> CR8 ===
>
> +# Testcase start: Invalidate streaming standby's slot as well as logical
> +# failover slot on primary due to inactive timeout GUC. Also, check the logical
>
> s/inactive timeout GUC/replication_slot_inactive_timeout/?

Done.

> CR9 ===
>
> +# Start: Helper functions used for this test file
> +# End: Helper functions used for this test file
>
> I think that's the first TAP test with this comment. Not saying we should not but
> why did you feel the need to add those?

Hm. Removed.

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

On Wed, Apr 3, 2024 at 2:58 PM shveta malik <[email-masked]> wrote:
>
> v31-002:
> (I had reviewed v29-002 but missed to post comments,  I think these
> are still applicable)
>
> 1) I think replication_slot_inactivity_timeout was recommended here
> (instead of replication_slot_inactive_timeout, so please give it a
> thought):
> https://www.postgresql.org/message-id/202403260739.udlp7lxixktx%40alvherre.pgsql

Yeah. It's synonymous with inactive_since. If others have an opinion
to have replication_slot_inactivity_timeout, I'm fine with it.

> 2) Commit msg:
> a)
> "It is often easy for developers to set a timeout of say 1
> or 2 or 3 days at slot level, after which the inactive slots get
> dropped."
>
> Shall we say invalidated rather than dropped?

Right. Done that.

> b)
> "To achieve the above, postgres introduces a GUC allowing users
> set inactive timeout and then a slot stays inactive for this much
> amount of time it invalidates the slot."
>
> Broken sentence.

Reworded it a bit.

Please find the attached v33 patches.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 3 Apr 2024 16:27:45 +0000

Hi,

> on inactive_since TAP tests.

Thanks! v33-0001 LGTM.

> recreated on the standby.

Yeah, right (I was confused with synced slots that are invalidated locally).

> However, I

CR1 ===

Yeah, I can see:

@@ -575,6 +575,13 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid)
                                                   " name slot \"%s\" already exists on the standby",
                                                   remote_slot->name));

+               /*
+                * Skip the sync if the local slot is already invalidated. We do this
+                * beforehand to save on slot acquire and release.
+                */
+               if (slot->data.invalidated != RS_INVAL_NONE)
+                       return false;

Thanks to the drop_local_obsolete_slots() call I think we are not missing the case
where the slot has been invalidated on the primary, invalidation reason has been
synced on the standby and later the slot is dropped/ recreated manually on the
primary (then it should be dropped/recreated on the standby too).

Also it seems we are not missing the case where a sync slot is invalidated
locally due to wal removal (it should be dropped/recreated).

> to something like the following, if others feel we don't need that, I

Sorry If I was not clear but I meant to say "Do you think it's worth to explain
why we decided to create a dedicated function"? (currently we "just" explain that
we created one).

Regards,

-- 
Bertrand Drouvot

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 4 Apr 2024 13:11:31 +0900

> Please find the attached v33 patches.

@@ -1368,6 +1416,7 @@ ShutDownSlotSync(void)
    if (SlotSyncCtx->pid == InvalidPid)
    {
        SpinLockRelease(&SlotSyncCtx->mutex);
+       update_synced_slots_inactive_since();
        return;
    }
    SpinLockRelease(&SlotSyncCtx->mutex);
@@ -1400,6 +1449,8 @@ ShutDownSlotSync(void)
    }

    SpinLockRelease(&SlotSyncCtx->mutex);
+
+   update_synced_slots_inactive_since();
 }

Why do we want to update all synced slots' inactive_since values at
shutdown in spite of updating the value every time when releasing the
slot? It seems to contradict the fact that inactive_since is updated
when releasing or restoring the slot.

Regards,

-- 
Masahiko Sawada

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 4 Apr 2024 10:04:02 +0530

> when releasing or restoring the slot.

It is to get the inactive_since right for the cases where the standby
is promoted without a restart similar to when a standby is promoted
with restart in which case the inactive_since is set to current time
in RestoreSlotFromDisk.

Imagine the slot is synced last time at time t1 and then a few hours
passed, the standby is promoted without a restart. If we don't set
inactive_since to current time in this case in ShutDownSlotSync, the
inactive timeout invalidation mechanism can kick in immediately.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 4 Apr 2024 10:48:11 +0530

> on inactive_since TAP tests.

The v33-0001 looks good to me. I have made minor changes in the
comments/commit message and removed one part of the test which was a
bit confusing and didn't seem to add much value. Let me know what you
think of the attached?

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 4 Apr 2024 11:12:11 +0530

> bit confusing and didn't seem to add much value. Let me know what you

Thanks for the changes. v34-0001 LGTM.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 4 Apr 2024 17:01:30 +0900

> inactive timeout invalidation mechanism can kick in immediately.

Thank you for the explanation! I understood the needs.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Thu, 4 Apr 2024 14:06:42 +0530

> Thank you for the explanation! I understood the needs.

Do you want to review the v34_0001* further or shall I proceed with
the commit of the same?

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 4 Apr 2024 18:05:02 +0900

> Do you want to review the v34_0001* further or shall I proceed with

Thanks for asking. The v34-0001 patch looks good to me.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Thu, 4 Apr 2024 16:35:01 +0530

> Thanks for the changes. v34-0001 LGTM.

I was doing a final review before pushing 0001 and found that
'inactive_since' could be set twice during startup after promotion,
once while restoring slots and then via ShutDownSlotSync(). The reason
is that ShutDownSlotSync() will be invoked in normal startup on
primary though it won't do anything apart from setting inactive_since
if we have synced slots. I think you need to check 'StandbyMode' in
update_synced_slots_inactive_since() and return if the same is not
set. We can't use 'InRecovery' flag as that will be set even during
crash recovery.

Can you please test this once unless you don't agree with the above theory?

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 4 Apr 2024 17:52:50 +0530

> Can you please test this once unless you don't agree with the above theory?

Nice catch. I've verified that update_synced_slots_inactive_since is
called even for normal server startups/crash recovery. I've added a
check to exit if the StandbyMode isn't set.

Please find the attached v35 patch.

---

From: shveta malik <[email-masked]>
Date: Fri, 5 Apr 2024 08:39:13 +0530

> Please find the attached v35 patch.

Thanks for the patch. Tested it , works well. Few cosmetic changes needed:

in 040 test file:
1)
# Capture the inactive_since of the slot from the primary. Note that the slot
# will be inactive since the corresponding subscription is disabled..

2 .. at the end. Replace with one.

2)
# Synced slot on the standby must get its own inactive_since.

. not needed in single line comment (to be consistent with
neighbouring comments)

3)
update_synced_slots_inactive_since():

if (!StandbyMode)
return;

It will be good to add comments here.

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 5 Apr 2024 11:21:43 +0530

Right.

> why we decided to create a dedicated function"? (currently we "just" explain that

We added a new function (InvalidateInactiveReplicationSlot) to
invalidate slot based on inactive timeout because 1) we do the
inactive timeout invalidation at slot level as opposed to
InvalidateObsoleteReplicationSlots which does loop over all the slots,
2)
InvalidatePossiblyObsoleteSlot does release the lock in some cases,
has a lot of unneeded code for inactive timeout invalidation check, 3)
we want some control over saving the slot to disk because we hook the
inactive timeout invalidation into the loop that checkpoints the slot
info to the disk in CheckPointReplicationSlots.

I've added a comment atop InvalidateInactiveReplicationSlot.

Please find the attached v36 patch.

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 5 Apr 2024 07:43:58 +0000

Hi,

> Please find the attached v36 patch.

Thanks!

A few comments:

1 ===

+       <para>
+        The timeout is measured from the time since the slot has become
+        inactive (known from its
+        <structfield>inactive_since</structfield> value) until it gets
+        used (i.e., its <structfield>active</structfield> is set to true).
+       </para>

That's right except when it's invalidated during the checkpoint (as the slot
is not acquired in CheckPointReplicationSlots()).

So, what about adding: "or a checkpoint occurs"? That would also explain that
the invalidation could occur during checkpoint.

2 ===

+       /* If the slot has been invalidated, recalculate the resource limits */
+       if (invalidated)
+       {

/If the slot/If a slot/?

3 ===

+ * NB - this function also runs as part of checkpoint, so avoid raising errors

s/NB - this/NB: This function/? (that looks more consistent with other comments
in the code)

4 ===

+ * Note that having a new function for RS_INVAL_INACTIVE_TIMEOUT cause instead

I understand it's "the RS_INVAL_INACTIVE_TIMEOUT cause" but reading "cause instead"
looks weird to me. Maybe it would make sense to reword this a bit.

5 ===

+        * considered not active as they don't actually perform logical decoding.

Not sure that's 100% accurate as we switched in fast forward logical
in 2ec005b4e2.

"as they perform only fast forward logical decoding (or not at all)", maybe?

6 ===

+       if (RecoveryInProgress() && slot->data.synced)
+               return false;
+
+       if (replication_slot_inactive_timeout == 0)
+               return false;

What about just using one if? It's more a matter of taste but it also probably
reduces the object file size a bit for non optimized build.

7 ===

+               /*
+                * Do not invalidate the slots which are currently being synced from
+                * the primary to the standby.
+                */
+               if (RecoveryInProgress() && slot->data.synced)
+                       return false;

I think we don't need this check as the exact same one is done just before.

8 ===

+sub check_for_slot_invalidation_in_server_log
+{
+       my ($node, $slot_name, $offset) = @_;
+       my $invalidated = 0;
+
+       for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
+       {
+               $node->safe_psql('postgres', "CHECKPOINT");

Wouldn't be better to wait for the replication_slot_inactive_timeout time before
instead of triggering all those checkpoints? (it could be passed as an extra arg
to wait_for_slot_invalidation()).

9 ===

# Synced slot mustn't get invalidated on the standby, it must sync invalidation
# from the primary. So, we must not see the slot's invalidation message in server
# log.
ok( !$standby1->log_contains(
        "invalidating obsolete replication slot \"lsub1_sync_slot\"",
        $standby1_logstart),
    'check that syned slot has not been invalidated on the standby');

Would that make sense to trigger a checkpoint on the standby before this test?
I mean I think that without a checkpoint on the standby we should not see the
invalidation in the log anyway.

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 6 Apr 2024 11:55:38 +0530

> the invalidation could occur during checkpoint.

Reworded.

> +       /* If the slot has been invalidated, recalculate the resource limits */

Modified it to be like elsewhere.

Done.

> looks weird to me. Maybe it would make sense to reword this a bit.

Reworded.

> "as they perform only fast forward logical decoding (or not at all)", maybe?

Changed it to "as they don't perform logical decoding to produce the
changes". In fast_forward mode no changes are produced.

> reduces the object file size a bit for non optimized build.

Changed.

> I think we don't need this check as the exact same one is done just before.

Right. Removed.

Done.

Done.

Please find the attached v37 patch for further review.

---

From: Amit Kapila <[email-masked]>
Date: Sat, 6 Apr 2024 12:18:34 +0530

On Sat, Apr 6, 2024 at 11:55 AM Bharath Rupireddy
<[email-masked]> wrote:

Why the handling w.r.t active_pid in InvalidatePossiblyInactiveSlot()
is not similar to InvalidatePossiblyObsoleteSlot(). Won't we need to
ensure that there is no other active slot user? Is it sufficient to
check inactive_since for the same? If so, we need some comments to
explain the same.

Can we avoid introducing the new functions like
SaveGivenReplicationSlot() and MarkGivenReplicationSlotDirty(), if we
do the required work in the caller?

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 6 Apr 2024 17:10:19 +0530

> check inactive_since for the same? If so, we need some comments to

I removed the separate functions and with minimal changes, I've now
placed the RS_INVAL_INACTIVE_TIMEOUT logic into
InvalidatePossiblyObsoleteSlot and use that even in
CheckPointReplicationSlots.

> do the required work in the caller?

Hm. Removed them now.

Please see the attached v38 patch.

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 13 Apr 2024 09:36:25 +0530

> Please see the attached v38 patch.

Hi, thanks everyone for reviewing the design and patches so far. Here
I'm with the v39 patches implementing inactive timeout based (0001)
and XID age based (0002) invalidation mechanisms.

I'm quoting the hackers who are okay with inactive timeout based
invalidation mechanism:
Bertrand Drouvot -
https://www.postgresql.org/message-id/ZgL0N%2BxVJNkyqsKL%40ip-10-97-1-34.eu-west-3.compute.internal
and https://www.postgresql.org/message-id/ZgPHDAlM79iLtGIH%40ip-10-97-1-34.eu-west-3.compute.internal
Amit Kapila - https://www.postgresql.org/message-id/CAA4eK1L3awyzWMuymLJUm8SoFEQe%3DDa9KUwCcAfC31RNJ1xdJA%40mail.gmail.com
Nathan Bossart -
https://www.postgresql.org/message-id/20240325195443.GA2923888%40nathanxps13
Robert Haas - https://www.postgresql.org/message-id/CA%2BTgmoZTbaaEjSZUG1FL0mzxAdN3qmXksO3O9_PZhEuXTkVnRQ%40mail.gmail.com

I'm quoting the hackers who are okay with XID age based invalidation mechanism:
Nathan Bossart -
https://www.postgresql.org/message-id/20240326150918.GB3181099%40nathanxps13
and https://www.postgresql.org/message-id/20240327150557.GA3994937%40nathanxps13
Alvaro Herrera -
https://www.postgresql.org/message-id/202403261539.xcjfle7sksz7%40alvherre.pgsql
Bertrand Drouvot -
https://www.postgresql.org/message-id/ZgPHDAlM79iLtGIH%40ip-10-97-1-34.eu-west-3.compute.internal
Amit Kapila - https://www.postgresql.org/message-id/CAA4eK1L3awyzWMuymLJUm8SoFEQe%3DDa9KUwCcAfC31RNJ1xdJA%40mail.gmail.com

There was a point raised by Robert
https://www.postgresql.org/message-id/CA%2BTgmoaRECcnyqxAxUhP5dk2S4HX%3DpGh-p-PkA3uc%2BjG_9hiMw%40mail.gmail.com
for XID age based invalidation. An issue related to
vacuum_defer_cleanup_age
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=be504a3e974d75be6f95c8f9b7367126034f2d12
led to the removal of the GUC
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1118cd37eb61e6a2428f457a8b2026a7bb3f801a.
The same issue may not happen for the XID age based invaliation. This
is because the XID age is not calculated using FullTransactionId but
using TransactionId as the slot's xmin and catalog_xmin are tracked as
TransactionId.

There was a point raised by Amit
https://www.postgresql.org/message-id/CAA4eK1K8wqLsMw6j0hE_SFoWAeo3Kw8UNnMfhsWaYDF1GWYQ%2Bg%40mail.gmail.com
on when to do the XID age based invalidation - whether in checkpointer
or when vacuum is being run or whenever ComputeXIDHorizons gets called
or in autovacuum process. For now, I've chosen the design to do these
new invalidation checks in two places - 1) whenever the slot is
acquired and the slot acquisition errors out if invalidated, 2) during
checkpoint. However, I'm open to suggestions on this.

I've also verified the case whether the replication_slot_xid_age
setting can help in case of server inching towards the XID wraparound.
I've created a primary and streaming standby setup with
hot_standby_feedback set to on (so that the slot gets an xmin). Then,
I've set replication_slot_xid_age to 2 billion on the primary, and
used xid_wraparound extension to reach XID wraparound on the primary.
Once I start receiving the WARNINGs about VACUUM, I did a checkpoint
after which the slot got invalidated enabling my VACUUM to freeze XIDs
saving my database from XID wraparound problem.

Thanks a lot Masahiko Sawada for an offlist chat about the XID age
calculation logic.

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 22 Apr 2024 22:50:37 +0900

Hi,

> Please find the attached v35 patch.

The documentation says about both 'active' and 'inactive_since'
columns of pg_replication_slots say:

---
active bool
True if this slot is currently actively being used

inactive_since timestamptz
The time since the slot has become inactive. NULL if the slot is
currently being used. Note that for slots on the standby that are
being synced from a primary server (whose synced field is true), the
inactive_since indicates the last synchronization (see Section 47.2.3)
time.
---

When reading the description I thought if 'active' is true,
'inactive_since' is NULL, but it doesn't seem to apply for temporary
slots. Since we don't reset the active_pid field of temporary slots
when the release, the 'active' is still true in the view but
'inactive_since' is not NULL. Do you think we need to mention it in
the documentation?

As for the timeout-based slot invalidation feature, we could end up
invalidating the temporary slots even if they are shown as active,
which could confuse users. Do we want to somehow deal with it?

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 25 Apr 2024 11:11:35 +0530

On Mon, Apr 22, 2024 at 7:21 PM Masahiko Sawada <[email-masked]> wrote:
> slots.

Right.

> Since we don't reset the active_pid field of temporary slots
> 'inactive_since' is not NULL.

Right. inactive_since is reset whenever the temporary slot is acquired
again within the same backend that created the temporary slot.

> Do you think we need to mention it in
> the documentation?

I think that's the reason we dropped "active" from the statement. It
was earlier "NULL if the slot is currently actively being used.". But,
per Bertrand's comment
https://www.postgresql.org/message-id/ZehE2IJcsetSJMHC%40ip-10-97-1-34.eu-west-3.compute.internal
changed it to ""NULL if the slot is currently being used.".

Temporary slots retain the active = true and active_pid = <pid of the
backend that created it> even when the slot is not being used until
the lifetime of the backend process. We haven't tied active or
active_pid flags to inactive_since, doing so now to represent the
temporary slot behaviour for active and active_pid will confuse users
more. As far as the inactive_since of a slot is concerned, it is set
to 0 when the slot is being used (acquired) and set to current
timestamp when the slot is not being used (released).

> which could confuse users. Do we want to somehow deal with it?

Yes. As long as the temporary slot is lying unused holding up
resources for more than the specified
replication_slot_inactive_timeout, it is bound to get invalidated.
This keeps behaviour consistent and less-confusing to the users.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 29 Apr 2024 09:33:28 +0530

On Thu, Apr 25, 2024 at 11:11 AM Bharath Rupireddy
<[email-masked]> wrote:
> more.

This is true and it's probably easy for us to understand as we
developed this feature but the same may not be true for others. I
wonder if we can be explicit about the difference of
active/inactive_since by adding something like the following for
inactive_since: Note that this field is not related to the active flag
as temporary slots can remain active till the session ends even when
they are not being used.

Sawada-San, do you have any suggestions on the wording?

 As far as the inactive_since of a slot is concerned, it is set
> This keeps behaviour consistent and less-confusing to the users.

Agreed. We may want to add something in the docs for this to avoid
confusion with the active flag.

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 17 Jun 2024 17:55:04 +0530

Hi,

> checkpoint. However, I'm open to suggestions on this.

Here are my thoughts on when to do the XID age invalidation. In all
the patches sent so far, the XID age invalidation happens in two
places - one during the slot acquisition, and another during the
checkpoint. As the suggestion is to do it during the vacuum (manual
and auto), so that even if the checkpoint isn't happening in the
database for whatever reasons, a vacuum command or autovacuum can
invalidate the slots whose XID is aged.

An idea is to check for XID age based invalidation for all the slots
in ComputeXidHorizons() before it reads replication_slot_xmin and
replication_slot_catalog_xmin, and obviously before the proc array
lock is acquired. A potential problem with this approach is that the
invalidation check can become too aggressive as XID horizons are
computed from many places.

Another idea is to check for XID age based invalidation for all the
slots in higher levels than ComputeXidHorizons(), for example in
vacuum() which is an entry point for both vacuum command and
autovacuum. This approach seems similar to vacuum_failsafe_age GUC
which checks each relation for the failsafe age before vacuum gets
triggered on it.

Does anyone see any issues or risks with the above two approaches or
have any other ideas? Thoughts?

I attached v40 patches here. I reworded some of the ERROR messages,
and did some code clean-up. Note that I haven't implemented any of the
above approaches yet.

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 17 Jun 2024 10:09:53 -0500

> invalidate the slots whose XID is aged.

+1.  IMHO this is a principled choice.  The similar max_slot_wal_keep_size
parameter is considered where it arguably matters most: when we are trying
to remove/recycle WAL segments.  Since this parameter is intended to
prevent the server from running out of space, it makes sense that we'd
apply it at the point where we are trying to free up space.  The proposed
max_slot_xid_age parameter is intended to prevent the server from running
out of transaction IDs, so it follows that we'd apply it at the point where
we reclaim them, which happens to be vacuum.

> which checks each relation for the failsafe age before vacuum gets

I don't presently have any strong opinion on where this logic should go,
but in general, I think we should only invalidate slots if invalidating
them would allow us to advance the vacuum cutoff.  If the cutoff is held
back by something else, I don't see a point in invalidating slots because
we'll just be breaking replication in return for no additional reclaimed
transaction IDs.

-- 
nathan

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 24 Jun 2024 11:30:00 +0530

Hi,

> which checks each relation for the failsafe age before vacuum gets

I am attaching the patches implementing the idea of invalidating
replication slots during vacuum when current slot xmin limits
(procArray->replication_slot_xmin and
procArray->replication_slot_catalog_xmin) are aged as per the new XID
age GUC. When either of these limits are aged, there must be at least
one replication slot that is aged, because the xmin limits, after all,
are the minimum of xmin or catalog_xmin of all replication slots. In
this approach, the new XID age GUC will help vacuum when needed,
because the current slot xmin limits are recalculated after
invalidating replication slots that are holding xmins for longer than
the age. The code is placed in vacuum() which is common for both
vacuum command and autovacuum, and gets executed only once every
vacuum cycle to not be too aggressive in invalidating.

However, there might be some concerns with this approach like the following:
1) Adding more code to vacuum might not be acceptable
2) What if invalidation of replication slots emits an error, will it
block vacuum forever? Currently, InvalidateObsoleteReplicationSlots()
is also called as part of the checkpoint, and emitting ERRORs from
within is avoided already. Therefore, there is no concern here for
now.
3) What if there are more replication slots to be invalidated, will it
delay the vacuum? If yes, by how much? <<TODO>>
4) Will the invalidation based on just current replication slot xmin
limits suffice irrespective of vacuum cutoffs? IOW, if the replication
slots are invalidated but vacuum isn't going to do any work because
vacuum cutoffs are not yet met? Is the invalidation work wasteful
here?
5) Is it okay to take just one more time the proc array lock to get
current replication slot xmin limits via
ProcArrayGetReplicationSlotXmin() once every vacuum cycle? <<TODO>>
6) Vacuum command can't be run on the standby in recovery. So, to help
invalidate replication slots on the standby, I have for now let the
checkpointer also do the XID age based invalidation. I know
invalidating both in checkpointer and vacuum may not be a great idea,
but I'm open to thoughts.

Following are some of the alternative approaches which IMHO don't help
vacuum when needed:
a) Let the checkpointer do the XID age based invalidation, and call it
out in the documentation that if the checkpoint doesn't happen, the
new GUC doesn't help even if the vacuum is run. This has been the
approach until v40 patch.
b) Checkpointer and/or other backends add an autovacuum work item via
AutoVacuumRequestWork(), and autovacuum when it gets to it will
invalidate the replication slots. But, what to do for the vacuum
command here?

Please find the attached v41 patches implementing the idea of vacuum
doing the invalidation.

Thoughts?

Thanks to Sawada-san for a detailed off-list discussion.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 9 Jul 2024 17:01:23 -0500

> but I'm open to thoughts.

Hm.  I hadn't considered this angle.

> approach until v40 patch.

My first reaction is that this is probably okay.  I guess you might run
into problems if you set max_slot_xid_age to 2B and checkpoint_timeout to 1
day, but even in that case your transaction ID usage rate would need to be
pretty high for wraparound to occur.

-- 
nathan

---

From: Ajin Cherian <[email-masked]>
Date: Mon, 12 Aug 2024 22:17:55 +1000

> Thanks to Sawada-san for a detailed off-list discussion.

The patch no longer applies on HEAD, please rebase.

regards,
Ajin Cherian
Fujitsu Australia

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 13 Aug 2024 06:32:45 +0900

> Hm.  I hadn't considered this angle.

Another idea would be to let the startup process do slot invalidation
when replaying a RUNNING_XACTS record. Since a RUNNING_XACTS record
has the latest XID on the primary, I think the startup process can
compare it to the slot-xmin, and invalidate slots which are older than
the age limit.

Regards,

---

From: Ajin Cherian <[email-masked]>
Date: Wed, 14 Aug 2024 13:50:38 +1000

> Please find the attached v41 patches implementing the idea of vacuum

Some minor comments on the patch:
1.
+ /*
+ * Release the lock if it's not yet to keep the cleanup path on
+ * error happy.
+ */

I suggest rephrasing to: " "Release the lock if it hasn't been already to
ensure smooth cleanup on error."

2.

elog(DEBUG1, "performing replication slot invalidation");

Probably change it to "performing replication slot invalidation checks" as
we might not actually invalidate any slot here.

3.

In CheckPointReplicationSlots()

+ invalidated =
InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT,
+ 0,
+ InvalidOid,
+ InvalidTransactionId);
+
+ if (invalidated)
+ {
+ /*
+ * If any slots have been invalidated, recalculate the resource
+ * limits.
+ */
+ ReplicationSlotsComputeRequiredXmin(false);
+ ReplicationSlotsComputeRequiredLSN();
+ }

Is this calculation of resource limits really required here when the same
is already done inside InvalidateObsoleteReplicationSlots()

regards,
Ajin Cherian
Fujitsu Australia

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 26 Aug 2024 11:44:05 +0530

> Some minor comments on the patch:

Thanks for reviewing.

> I suggest rephrasing to: " "Release the lock if it hasn't been already to ensure smooth cleanup on error."

Changed.

> Probably change it to "performing replication slot invalidation checks" as we might not actually invalidate any slot here.

Changed.

> Is this calculation of resource limits really required here when the same is already done inside InvalidateObsoleteReplicationSlots()

Nice catch. Removed.

Please find the attached v42 patches.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 26 Aug 2024 16:35:02 +0530

On Mon, Aug 26, 2024 at 11:44 AM Bharath Rupireddy
<[email-masked]> wrote:

Few comments on 0001:
1.
@@ -651,6 +651,13 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid
remote_dbid)
     " name slot \"%s\" already exists on the standby",
     remote_slot->name));

+ /*
+ * Skip the sync if the local slot is already invalidated. We do this
+ * beforehand to avoid slot acquire and release.
+ */
+ if (slot->data.invalidated != RS_INVAL_NONE)
+ return false;
+
  /*
  * The slot has been synchronized before.

I was wondering why you have added this new check as part of this
patch. If you see the following comments in the related code, you will
know why we haven't done this previously.

/*
* The slot has been synchronized before.
*
* It is important to acquire the slot here before checking
* invalidation. If we don't acquire the slot first, there could be a
* race condition that the local slot could be invalidated just after
* checking the 'invalidated' flag here and we could end up
* overwriting 'invalidated' flag to remote_slot's value. See
* InvalidatePossiblyObsoleteSlot() where it invalidates slot directly
* if the slot is not acquired by other processes.
*
* XXX: If it ever turns out that slot acquire/release is costly for
* cases when none of the slot properties is changed then we can do a
* pre-check to ensure that at least one of the slot properties is
* changed before acquiring the slot.
*/
ReplicationSlotAcquire(remote_slot->name, true);

We need some modifications in these comments if you want to add a
pre-check here.

2.
@@ -1907,6 +2033,31 @@ CheckPointReplicationSlots(bool is_shutdown)
  SaveSlotToPath(s, path, LOG);
  }
  LWLockRelease(ReplicationSlotAllocationLock);
+
+ elog(DEBUG1, "performing replication slot invalidation checks");
+
+ /*
+ * Note that we will make another pass over replication slots for
+ * invalidations to keep the code simple. The assumption here is that the
+ * traversal over replication slots isn't that costly even with hundreds
+ * of replication slots. If it ever turns out that this assumption is
+ * wrong, we might have to put the invalidation check logic in the above
+ * loop, for that we might have to do the following:
+ *
+ * - Acqure ControlLock lock once before the loop.
+ *
+ * - Call InvalidatePossiblyObsoleteSlot for each slot.
+ *
+ * - Handle the cases in which ControlLock gets released just like
+ * InvalidateObsoleteReplicationSlots does.
+ *
+ * - Avoid saving slot info to disk two times for each invalidated slot.
+ *
+ * XXX: Should we move inactive_timeout inavalidation check closer to
+ * wal_removed in CreateCheckPoint and CreateRestartPoint?
+ */
+ InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT,
+    0, InvalidOid, InvalidTransactionId);

Why do we want to call this for shutdown case (when is_shutdown is
true)? I understand trying to invalidate slots during regular
checkpoint but not sure if we need it at the time of shutdown. The
other point is can we try to check the performance impact with 100s of
slots as mentioned in the code comments?

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 29 Aug 2024 11:31:09 +0530

Hi,

Thanks for looking into this.

> know why we haven't done this previously.

Removed. Can deal with optimization separately.

> checkpoint but not sure if we need it at the time of shutdown.

Changed it to invalidate only for non-shutdown checkpoints.
inactive_timeout invalidation isn't critical for shutdown unlike
wal_removed which can help shutdown by freeing up some disk space.

> The

I first checked how much does the wal_removed invalidation check add to the
checkpoint (see 2nd and 3rd column). I then checked how much
inactive_timeout invalidation check adds to the checkpoint (see 4th
column), it is not more than wal_remove invalidation check. I then checked
how much the wal_removed invalidation check adds for replication slots that
have already been invalidated due to inactive_timeout (see 5th column),
looks like not much.

| # of slots | HEAD (no invalidation) ms | HEAD (wal_removed) ms | PATCHED
(inactive_timeout) ms | PATCHED (inactive_timeout+wal_removed) ms |
|------------|----------------------------|-----------------------|-------------------------------|------------------------------------------|
| 100        | 18.591                     | 370.586               | 359.299
                      | 373.882                                  |
| 1000       | 15.722                     | 4834.901              |
5081.751                      | 5072.128                                 |
| 10000      | 19.261                     | 59801.062             |
61270.406                     | 60270.099                                |

Having said that, I'm okay to implement the optimization specified.
Thoughts?

+ /*
+ * NB: We will make another pass over replication slots for
+ * invalidation checks to keep the code simple. Testing shows that
+ * there is no noticeable overhead (when compared with wal_removed
+ * invalidation) even if we were to do inactive_timeout invalidation
+ * of thousands of replication slots here. If it is ever proven that
+ * this assumption is wrong, we will have to perform the invalidation
+ * checks in the above for loop with the following changes:
+ *
+ * - Acquire ControlLock lock once before the loop.
+ *
+ * - Call InvalidatePossiblyObsoleteSlot for each slot.
+ *
+ * - Handle the cases in which ControlLock gets released just like
+ * InvalidateObsoleteReplicationSlots does.
+ *
+ * - Avoid saving slot info to disk two times for each invalidated
+ * slot.

Please see the attached v43 patches addressing the above review comments.

---

From: Peter Smith <[email-masked]>
Date: Fri, 30 Aug 2024 12:43:10 +1000

Hi, here are some review comments for patch v43-0001.

======
Commit message

1.
... introduces a GUC allowing users set inactive timeout.

~

1a. You should give the name of the new GUC in the commit message.

1b. /set/to set/

======
doc/src/sgml/config.sgml

GUC "replication_slot_inactive_timeout"

2.
Invalidates replication slots that are inactive for longer than
specified amount of time

nit - suggest use similar wording as the prior GUC (wal_sender_timeout):
Invalidate replication slots that are inactive for longer than this
amount of time.

~

3.
This invalidation check happens either when the slot is acquired for
use or during a checkpoint. The time since the slot has become
inactive is known from its inactive_since value using which the
timeout is measured.

nit - the wording is too complicated. suggest:
The timeout check occurs when the slot is next acquired for use, or
during a checkpoint. The slot's 'inactive_since' field value is when
the slot became inactive.

~

4.
Note that the inactive timeout invalidation mechanism is not
applicable for slots on the standby that are being synced from a
primary server (whose synced field is true).

nit - that word "whose" seems ambiguous. suggest:
(e.g. the standby slot has 'synced' field true).

======
doc/src/sgml/system-views.sgml

5.
inactive_timeout means that the slot has been inactive for the
duration specified by replication_slot_inactive_timeout parameter.

nit - suggestion ("longer than"):
... the slot has been inactive for longer than the duration specified
by the replication_slot_inactive_timeout parameter.

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

6.
 /* Maximum number of invalidation causes */
-#define RS_INVAL_MAX_CAUSES RS_INVAL_WAL_LEVEL
+#define RS_INVAL_MAX_CAUSES RS_INVAL_INACTIVE_TIMEOUT

IMO this #define belongs in the slot.h, immediately below where the
enum is defined.

~~~

7. ReplicationSlotAcquire:

I had a fundamental question about this logic.

IIUC the purpose of the patch was to invalidate replication slots that
have been inactive for too long.

So, it makes sense to me that some periodic processing (e.g.
CheckPointReplicationSlots) might do a sweep over all the slots, and
invalidate the too-long-inactive ones that it finds.

OTOH, it seemed quite strange to me that the patch logic is also
detecting and invalidating inactive slots during the
ReplicationSlotAcquire function. This is kind of saying "ERROR -
sorry, because this was inactive for too long you can't have it" at
the very moment that you wanted to use it again! IIUC such a slot
would be invalidated by the function InvalidatePossiblyObsoleteSlot(),
but therein lies my doubt -- how can the slot be considered as
"obsolete" when we are in the very act of trying to acquire/use it?

I guess it might be argued this is not so different to the scenario of
attempting to acquire a slot that had been invalidated momentarily
before during checkpoint processing. But, somehow that scenario seems
more like bad luck to me, versus ReplicationSlotAcquire() deliberately
invalidating something we *know* is wanted.

~

8.
+ 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 was inactive
since %s for more than %d seconds specified by
\"replication_slot_inactive_timeout\".",
+    timestamptz_to_str(s->inactive_since),
+    replication_slot_inactive_timeout)));

nit - IMO the info should be split into errdetail + errhint. Like this:
errdetail("The slot became invalid because it was inactive since %s,
which is more than %d seconds ago."...)
errhint("You might need to increase \"%s\".",
"replication_slot_inactive_timeout")

~~~

9. ReportSlotInvalidation

+ appendStringInfo(&err_detail,
+ _("The slot has been inactive since %s for more than %d seconds
specified by \"replication_slot_inactive_timeout\"."),
+ timestamptz_to_str(inactive_since),
+ replication_slot_inactive_timeout);
+ break;

IMO this error in ReportSlotInvalidation() should be the same as the
other one from ReplicationSlotAcquire(), which I suggested above
(comment #8) should include a hint. Also, including a hint here will
make this new message consistent with the other errhint (for
"max_slot_wal_keep_size") that is already in this function.

~~~

10. InvalidatePossiblyObsoleteSlot

+ if (cause == RS_INVAL_INACTIVE_TIMEOUT &&
+ (replication_slot_inactive_timeout > 0 &&
+ s->inactive_since > 0 &&
+ !(RecoveryInProgress() && s->data.synced)))

10a. Everything here is && so this has some redundant parentheses.

10b. Actually, IMO this complicated condition is overkill. Won't it be
better to just unconditionally assign
now = GetCurrentTimestamp(); here?

~

11.
+ * Note that we don't invalidate synced slots because,
+ * they are typically considered not active as they don't
+ * perform logical decoding to produce the changes.

nit - tweaked punctuation

~

12.
+ * If the slot can be acquired, do so or if the slot is already ours,
+ * then mark it invalidated.  Otherwise we'll signal the owning
+ * process, below, and retry.

nit - tidied this comment. Suggestion:
If the slot can be acquired, do so and mark it as invalidated. If the
slot is already ours, mark it as invalidated. Otherwise, we'll signal
the owning process below and retry.

~

13.
+ if (active_pid == 0 ||
+ (MyReplicationSlot != NULL &&
+ MyReplicationSlot == s &&
+ active_pid == MyProcPid))

You are already checking MyReplicationSlot == s here, so that extra
check for MyReplicationSlot != NULL is redundant, isn't it?

~~~

14. CheckPointReplicationSlots

 /*
- * Flush all replication slots to disk.
+ * Flush all replication slots to disk. Also, invalidate 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

nit - /Also, invalidate slots/Also, invalidate obsolete slots/

======
src/backend/utils/misc/guc_tables.c

15.
+ {"replication_slot_inactive_timeout", PGC_SIGHUP, REPLICATION_SENDING,
+ gettext_noop("Sets the amount of time to wait before invalidating an "
+ "inactive replication slot."),

nit - that is maybe a bit misleading because IIUC there is no real
"waiting" happening anywhere. Suggest:
Sets the amount of time a replication slot can remain inactive before
it will be invalidated.

======

Please take a look at the attached top-up patches. These include
changes for many of the nits above.

======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 7009350..c96ae53 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -671,9 +671,10 @@ retry:
 					(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 was inactive since %s for more than %d seconds specified by \"replication_slot_inactive_timeout\".",
+					 errdetail("The slot became invalid because it was inactive since %s, which is more than %d seconds ago.",
 							   timestamptz_to_str(s->inactive_since),
-							   replication_slot_inactive_timeout)));
+							   replication_slot_inactive_timeout),
+					 errhint("You might need to increase \"%s\".", "replication_slot_inactive_timeout")));
 		}
 	}
 
@@ -1738,9 +1739,9 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
 					 * is disabled or slot is currently being used or the slot
 					 * on standby is currently being synced from the primary.
 					 *
-					 * Note that we don't invalidate synced slots because,
-					 * they are typically considered not active as they don't
-					 * perform logical decoding to produce the changes.
+					 * Note that we don't invalidate synced slots because
+					 * they are typically considered not active, as they don't
+					 * perform logical decoding to produce changes.
 					 */
 					if (replication_slot_inactive_timeout == 0 ||
 						s->inactive_since == 0 ||
@@ -1789,9 +1790,9 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
 		active_pid = s->active_pid;
 
 		/*
-		 * If the slot can be acquired, do so or if the slot is already ours,
-		 * then mark it invalidated.  Otherwise we'll signal the owning
-		 * process, below, and retry.
+		 * If the slot can be acquired, do so and mark it as invalidated.
+		 * If the slot is already ours, mark it as invalidated.
+		 * Otherwise, we'll signal the owning process below and retry.
 		 */
 		if (active_pid == 0 ||
 			(MyReplicationSlot != NULL &&
@@ -1975,7 +1976,7 @@ restart:
 }
 
 /*
- * Flush all replication slots to disk. Also, invalidate slots during
+ * 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.
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 861692c..73a5824 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -3030,8 +3030,9 @@ struct config_int ConfigureNamesInt[] =
 
 	{
 		{"replication_slot_inactive_timeout", PGC_SIGHUP, REPLICATION_SENDING,
-			gettext_noop("Sets the amount of time to wait before invalidating an "
-						 "inactive replication slot."),
+
+			gettext_noop("Sets the amount of time a replication slot can remain "
+						 "inactive before it will be invalidated."),
 			NULL,
 			GUC_UNIT_S
 		},
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index fbbacbe..bd3ce5a 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4568,8 +4568,8 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
       </term>
       <listitem>
        <para>
-        Invalidates replication slots that are inactive for longer than
-        specified amount of time. If this value is specified without units,
+        Invalidate replication slots that are inactive for longer than this
+        amount of time. If this value is specified without units,
         it is taken as seconds. A value of zero (which is default) disables
         the timeout mechanism. This parameter can only be set in
         the <filename>postgresql.conf</filename> file or on the server
@@ -4577,18 +4577,16 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        </para>
 
        <para>
-        This invalidation check happens either when the slot is acquired
-        for use or during a checkpoint. The time since the slot has become
-        inactive is known from its
-        <structfield>inactive_since</structfield> value using which the
-        timeout is measured.
+        The timeout check occurs when the slot is next acquired for use, or
+        during a checkpoint. The slot's <structfield>inactive_since</structfield>
+        field value is when the slot became inactive.
        </para>
 
        <para>
         Note that the inactive timeout invalidation mechanism is not
         applicable for slots on the standby that are being synced from a
-        primary server (whose <structfield>synced</structfield> field is
-        <literal>true</literal>).
+        primary server (e.g. the standby slot <structfield>synced</structfield>
+        field is true).
        </para>
       </listitem>
      </varlistentry>
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 9e00f7d..f230e6e 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2621,7 +2621,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
         <listitem>
          <para>
           <literal>inactive_timeout</literal> means that the slot has been
-          inactive for the duration specified by
+          inactive for longer than the duration specified by
           <xref linkend="guc-replication-slot-inactive-timeout"/> parameter.
          </para>
         </listitem>

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 31 Aug 2024 13:45:39 +0530

Hi,

> 1a. You should give the name of the new GUC in the commit message.

Modified.

> 1b. /set/to set/

Reworded the commit message.

> Invalidate replication slots that are inactive for longer than this

Modified.

Reworded.

> by the replication_slot_inactive_timeout parameter.

Modified.

> IMO this #define belongs in the slot.h, immediately below where the

Please check the commit that introduced it -
https://www.postgresql.org/message-id/ZdU3CHqza9XJw4P-%40paquier.xyz.
It is kept in the file where it's used.

> invalidating something we *know* is wanted.

Hm. TBH, there's no real reason for invalidating the slot in
ReplicationSlotAcquire(). My thinking back then was to take this
opportunity to do some work. I agree to leave the invalidation work to
the checkpointer. However, I still think ReplicationSlotAcquire()
should error out if the slot has already been invalidated similar to
"can no longer get changes from replication slot \"%s\" for
wal_removed.

> "replication_slot_inactive_timeout")

"invalid" is being covered by errmsg "invalidating obsolete
replication slot", so no need to duplicate it in errdetail.

> "max_slot_wal_keep_size") that is already in this function.

Not exactly the same but similar. Because ReportSlotInvalidation()
errmsg has an "invalidating" component, whereas errmsg in
ReplicationSlotAcquire doesn't. Please check latest wordings.

> 10a. Everything here is && so this has some redundant parentheses.

Removed.

> now = GetCurrentTimestamp(); here?

GetCurrentTimestamp() can get costlier on certain platforms. I think
the fields checking in the condition are pretty straight forward -
e.g. !RecoveryInProgress() server not in recovery, !s->data.synced
slot is not being synced and so on. Added a macro
IsInactiveTimeoutSlotInvalidationApplicable() for better readability
in two places.

> nit - tweaked punctuation

Used the consistent wording in the commit message, docs and code comments.

> the owning process below and retry.

Modified.

> check for MyReplicationSlot != NULL is redundant, isn't it?

Removed.

> nit - /Also, invalidate slots/Also, invalidate obsolete slots/

Modified.

> Sets the amount of time a replication slot can remain inactive before

Modified.

Please find the attached v44 patch with the above changes. I will
include the 0002 xid_age based invalidation patch later.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 2 Sep 2024 12:25:35 +0530

On Thu, Aug 29, 2024 at 11:31 AM Bharath Rupireddy
<[email-masked]> wrote:
> Changed it to invalidate only for non-shutdown checkpoints. inactive_timeout invalidation isn't critical for shutdown unlike wal_removed which can help shutdown by freeing up some disk space.
>
> > The
> I first checked how much does the wal_removed invalidation check add to the checkpoint (see 2nd and 3rd column). I then checked how much inactive_timeout invalidation check adds to the checkpoint (see 4th column), it is not more than wal_remove invalidation check. I then checked how much the wal_removed invalidation check adds for replication slots that have already been invalidated due to inactive_timeout (see 5th column), looks like not much.
>
> | # of slots | HEAD (no invalidation) ms | HEAD (wal_removed) ms | PATCHED (inactive_timeout) ms | PATCHED (inactive_timeout+wal_removed) ms |
> | 100        | 18.591                     | 370.586               | 359.299                       | 373.882                                  |
> | 1000       | 15.722                     | 4834.901              | 5081.751                      | 5072.128                                 |
> | 10000      | 19.261                     | 59801.062             | 61270.406                     | 60270.099                                |
>
> Having said that, I'm okay to implement the optimization specified. Thoughts?
>

The other possibility is to try invalidating due to timeout along with
wal_removed case during checkpoint. The idea is that if the slot can
be invalidated due to WAL then fine, otherwise check if it can be
invalidated due to timeout. This can avoid looping the slots and doing
similar work multiple times during the checkpoint.

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Mon, 2 Sep 2024 18:06:44 +1000

Hi. Thanks for addressing my previous review comments.

Here are some review comments for v44-0001.

======
Commit message.

1.
Because such synced slots are typically considered not
active (for them to be later considered as inactive) as they don't
perform logical decoding to produce the changes.

~

This sentence is bad grammar. The docs have the same wording, so
please see my doc review comment #4 suggestion below.

======
doc/src/sgml/config.sgml

2.
+       <para>
+        Invalidates replication slots that are inactive for longer than
+        specified amount of time. If this value is specified without units,
+        it is taken as seconds. A value of zero (which is default) disables
+        the timeout mechanism. This parameter can only be set in
+        the <filename>postgresql.conf</filename> file or on the server
+        command line.
+       </para>
+

nit - This is OK as-is, but OTOH why not make the wording consistent
with the previous GUC description? (e.g. see my v43 [1] #2 review
comment)

~~~

3.
+       <para>
+        This invalidation check happens either when the slot is acquired
+        for use or during checkpoint. The time since the slot has become
+        inactive is known from its
+        <structfield>inactive_since</structfield> value using which the
+        timeout is measured.
+       </para>
+

I felt this is slightly misleading because slot acquiring has nothing
to do with setting the slot invalidation anymore. Furthermore, the 2nd
sentence is bad grammar.

nit - IMO something simple like the following rewording can address
both of those points:

Slot invalidation due to inactivity timeout occurs during checkpoint.
The duration of slot inactivity is calculated using the slot's
<structfield>inactive_since</structfield> field value.

~

4.
+        Because such synced slots are typically considered not active
+        (for them to be later considered as inactive) as they don't perform
+        logical decoding to produce the changes.

That sentence has bad grammar.

nit – suggest a much simpler replacement:
Synced slots are always considered to be inactive because they don't
perform logical decoding to produce changes.

======

5.
+#define IsInactiveTimeoutSlotInvalidationApplicable(s) \
+ (replication_slot_inactive_timeout > 0 && \
+ s->inactive_since > 0 && \
+ !RecoveryInProgress() && \
+ !s->data.synced)
+

5a.
I felt this would be better implemented as an inline function. Then it
can be commented on properly to explain the parts of the condition.
e.g. the large comment currently in InvalidatePossiblyObsoleteSlot()
would be more appropriate in this function.

~

5b.
The name is very long. Can't it be something shorter/simpler like:
'IsSlotATimeoutCandidate()'

~~~

6. ReplicationSlotAcquire

-ReplicationSlotAcquire(const char *name, bool nowait)
+ReplicationSlotAcquire(const char *name, bool nowait,
+    bool check_for_invalidation)

nit - Previously this new parameter really did mean to "check" for
[and set the slot] invalidation. But now I suggest renaming it to
'error_if_invalid' to properly reflect the new usage. And also in the
slot.h.

~

7.
+ /*
+ * Error out if the slot has been invalidated previously. Because there's
+ * no use in acquiring the invalidated slot.
+ */

nit - The comment is contrary to the code. If there was no reason to
skip this error, then you would not have the new parameter allowing
you to skip this error. I suggest just repeating the same comment as
in the function header.

~~~

8. ReportSlotInvalidation

nit - Added some blank lines for consistency.

~~~

9. InvalidatePossiblyObsoleteSlot

+ /*
+ * Quick exit if inactive timeout invalidation mechanism
+ * is disabled or slot is currently being used or the
+ * server is in recovery mode or the slot on standby is
+ * currently being synced from the primary.
+ *
+ * Note that the inactive timeout invalidation mechanism
+ * is not applicable for slots on the standby server that
+ * are being synced from primary server. Because such
+ * synced slots are typically considered not active (for
+ * them to be later considered as inactive) as they don't
+ * perform logical decoding to produce the changes.
+ */
+ if (!IsInactiveTimeoutSlotInvalidationApplicable(s))
+ break;

9a.
Consistency is good (commit message, docs and code comments for this),
but the added sentence has bad grammar. Please see the docs review
comment #4 above for some alternate phrasing.

~

9b.
Now that this logic is moved into a macro (I suggested it should be an
inline function) IMO this comment does not belong here anymore because
it is commenting code that you cannot see. Instead, this comment (or
something like it) should be as comments within the new function.

======
src/include/replication/slot.h

10.
+extern void ReplicationSlotAcquire(const char *name, bool nowait,
+    bool check_for_invalidation);

Change the new param name as described in the earlier review comment.

======
src/test/recovery/t/050_invalidate_slots.pl

~~~

Please refer to the attached file which implements some of the nits
mentioned above.

======
[1] v43 review -
https://www.postgresql.org/message-id/CAHut%2BPuFzCHPCiZbpoQX59kgZbebuWT0gR0O7rOe4t_sdYu%3DOA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 970b496..0537714 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4564,8 +4564,8 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
       </term>
       <listitem>
        <para>
-        Invalidates replication slots that are inactive for longer than
-        specified amount of time. If this value is specified without units,
+        Invalidate replication slots that are inactive for longer than this
+        amount of time. If this value is specified without units,
         it is taken as seconds. A value of zero (which is default) disables
         the timeout mechanism. This parameter can only be set in
         the <filename>postgresql.conf</filename> file or on the server
@@ -4573,11 +4573,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        </para>
 
        <para>
-        This invalidation check happens either when the slot is acquired
-        for use or during checkpoint. The time since the slot has become
-        inactive is known from its
-        <structfield>inactive_since</structfield> value using which the
-        timeout is measured.
+        Slot invalidation due to inactivity timeout occurs during checkpoint.
+        The duration of slot inactivity is calculated using the slot's
+        <structfield>inactive_since</structfield> field value.
        </para>
 
        <para>
@@ -4585,9 +4583,8 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
         applicable for slots on the standby server that are being synced
         from primary server (i.e., standby slots having
         <structfield>synced</structfield> field <literal>true</literal>).
-        Because such synced slots are typically considered not active
-        (for them to be later considered as inactive) as they don't perform
-        logical decoding to produce the changes.
+        Synced slots are always considered to be inactive because they don't
+        perform logical decoding to produce changes.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index acc0370..bb06592 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -551,12 +551,11 @@ ReplicationSlotName(int index, Name name)
  * An error is raised if nowait is true and the slot is currently in use. If
  * nowait is false, we sleep until the slot is released by the owning process.
  *
- * An error is raised if check_for_invalidation is true and the slot has been
+ * An error is raised if error_if_invalid is true and the slot has been
  * invalidated previously.
  */
 void
-ReplicationSlotAcquire(const char *name, bool nowait,
-					   bool check_for_invalidation)
+ReplicationSlotAcquire(const char *name, bool nowait,  bool error_if_invalid)
 {
 	ReplicationSlot *s;
 	int			active_pid;
@@ -635,11 +634,10 @@ retry:
 	MyReplicationSlot = s;
 
 	/*
-	 * Error out if the slot has been invalidated previously. Because there's
-	 * no use in acquiring the invalidated slot.
+	 * An error is raised if error_if_invalid is true and the slot has been
+	 * invalidated previously.
 	 */
-	if (check_for_invalidation &&
-		s->data.invalidated == RS_INVAL_INACTIVE_TIMEOUT)
+	if (error_if_invalid && s->data.invalidated == RS_INVAL_INACTIVE_TIMEOUT)
 	{
 		Assert(s->inactive_since > 0);
 		ereport(ERROR,
@@ -1565,6 +1563,7 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause,
 								 _("You might need to increase \"%s\"."), "max_slot_wal_keep_size");
 				break;
 			}
+
 		case RS_INVAL_HORIZON:
 			appendStringInfo(&err_detail, _("The slot conflicted with xid horizon %u."),
 							 snapshotConflictHorizon);
@@ -1573,6 +1572,7 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause,
 		case RS_INVAL_WAL_LEVEL:
 			appendStringInfoString(&err_detail, _("Logical decoding on standby requires \"wal_level\" >= \"logical\" on the primary server."));
 			break;
+
 		case RS_INVAL_INACTIVE_TIMEOUT:
 			Assert(inactive_since > 0);
 			appendStringInfo(&err_detail,
@@ -1584,6 +1584,7 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause,
 			appendStringInfo(err_hint,
 							 _("You might need to increase \"%s\"."), "replication_slot_inactive_timeout");
 			break;
+
 		case RS_INVAL_NONE:
 			pg_unreachable();
 	}
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index 431cc08..5678e1a 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -253,7 +253,7 @@ extern void ReplicationSlotAlter(const char *name, const bool *failover,
 								 const bool *two_phase);
 
 extern void ReplicationSlotAcquire(const char *name, bool nowait,
-								   bool check_for_invalidation);
+								   bool error_if_invalid);
 extern void ReplicationSlotRelease(void);
 extern void ReplicationSlotCleanup(bool synced_only);
 extern void ReplicationSlotSave(void);

---

From: Amit Kapila <[email-masked]>
Date: Mon, 2 Sep 2024 15:20:06 +0530

> include the 0002 xid_age based invalidation patch later.

It is better to get the 0001 reviewed and committed first. We can
discuss about 0002 afterwards as 0001 is in itself a complete and
separate patch that can be committed.

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Tue, 3 Sep 2024 16:55:58 +1000

Hi, my previous review posts did not cover the test code.

Here are my review comments for the v44-0001 test code

======
TEST CASE #1

1.
+# Wait for the inactive replication slot to be invalidated.
+$standby1->poll_query_until(
+ 'postgres', qq[
+ SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+ WHERE slot_name = 'lsub1_sync_slot' AND
+ invalidation_reason = 'inactive_timeout';
+])
+  or die
+  "Timed out while waiting for lsub1_sync_slot invalidation to be
synced on standby";
+

Is that comment correct? IIUC the synced slot should *already* be
invalidated from the primary, so here we are not really "waiting" for
it to be invalidated; Instead, we are just "confirming" that the
synchronized slot is already invalidated with the correct reason as
expected.

~~~

2.
+# Synced slot mustn't get invalidated on the standby even after a checkpoint,
+# it must sync invalidation from the primary. So, we must not see the slot's
+# invalidation message in server log.
+$standby1->safe_psql('postgres', "CHECKPOINT");
+ok( !$standby1->log_contains(
+ "invalidating obsolete replication slot \"lsub1_sync_slot\"",
+ $standby1_logstart),
+ 'check that syned lsub1_sync_slot has not been invalidated on the standby'
+);
+

This test case seemed bogus, for a couple of reasons:

2a. IIUC this 'lsub1_sync_slot' is the same one that is already
invalid (from the primary), so nobody should be surprised that an
already invalid slot doesn't get flagged as invalid again. i.e.
Shouldn't your test scenario here be done using a valid synced slot?

2b. AFAICT it was only moments above this CHECKPOINT where you
assigned the standby inactivity timeout to 2s. So even if there was
some bug invalidating synced slots I don't think you gave it enough
time to happen -- e.g. I doubt 2s has elapsed yet.

~

3.
+# Stop standby to make the standby's replication slot on the primary inactive
+$standby1->stop;
+
+# Wait for the standby's replication slot to become inactive
+wait_for_slot_invalidation($primary, 'sb1_slot', $logstart,
+ $inactive_timeout);

This seems a bit tricky. Both these (the stop and the wait) seem to
belong together, so I think maybe a single bigger explanatory comment
covering both parts would help for understanding.

======
TEST CASE #2

4.
+# Stop subscriber to make the replication slot on publisher inactive
+$subscriber->stop;
+
+# Wait for the replication slot to become inactive and then invalidated due to
+# timeout.
+wait_for_slot_invalidation($publisher, 'lsub1_slot', $logstart,
+ $inactive_timeout);

IIUC, this is just like comment #3 above. Both these (the stop and the
wait) seem to belong together, so I think maybe a single bigger
explanatory comment covering both parts would help for understanding.

~~~

5.
+# Testcase end: Invalidate logical subscriber's slot due to
+# replication_slot_inactive_timeout.
+# =============================================================================

IMO the rest of the comment after "Testcase end" isn't very useful.

======
sub wait_for_slot_invalidation

6.
+sub wait_for_slot_invalidation
+{

An explanatory header comment for this subroutine would be helpful.

~~~

7.
+ # Wait for the replication slot to become inactive
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+ WHERE slot_name = '$slot_name' AND active = 'f';
+ ])
+   or die
+   "Timed out while waiting for slot $slot_name to become inactive on
node $name";
+
+ # Wait for the replication slot info to be updated
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+ WHERE inactive_since IS NOT NULL
+ AND slot_name = '$slot_name' AND active = 'f';
+ ])
+   or die
+   "Timed out while waiting for info of slot $slot_name to be updated
on node $name";
+

Why are there are 2 separate poll_query_until's here? Can't those be
combined into just one?

~~~

8.
+ # Sleep at least $inactive_timeout duration to avoid multiple checkpoints
+ # for the slot to get invalidated.
+ sleep($inactive_timeout);
+

Maybe this special sleep to prevent too many CHECKPOINTs should be
moved to be inside the other subroutine, which is actually doing those
CHECKPOINTs.

~~~

9.
+ # Wait for the inactive replication slot to be invalidated
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+ WHERE slot_name = '$slot_name' AND
+ invalidation_reason = 'inactive_timeout';
+ ])
+   or die
+   "Timed out while waiting for inactive slot $slot_name to be
invalidated on node $name";
+

The comment seems misleading. IIUC you are not "waiting" for the
invalidation here, because it is the other subroutine doing the
waiting for the invalidation message in the logs. Instead, here I
think you are just confirming the 'invalidation_reason' got set
correctly. The comment should say what it is really doing.

======
sub check_for_slot_invalidation_in_server_log

10.
+# Check for invalidation of slot in server log
+sub check_for_slot_invalidation_in_server_log
+{

I think the main function of this subroutine is the CHECKPOINT and the
waiting for the server log to say invalidation happened. It is doing a
loop of a) CHECKPOINT then b) inspecting the server log for the slot
invalidation, and c) waiting for a bit. Repeat 10 times.

A comment describing the logic for this subroutine would be helpful.

The most important side-effect of this function is the CHECKPOINT
because without that nothing will ever get invalidated due to
inactivity, but this key point is not obvious from the subroutine
name.

IMO it would be better to name this differently to reflect what it is
really doing:
e.g. "CHECKPOINT_and_wait_for_slot_invalidation_in_server_log"

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

---

From: shveta malik <[email-masked]>
Date: Tue, 3 Sep 2024 15:01:06 +0530

> include the 0002 xid_age based invalidation patch later.

Thanks for the patch Bharath. My review and testing is WIP, but please
find few comments and queries:

1)
I see that ReplicationSlotAlter() will error out if the slot is
invalidated due to timeout. I have not tested it myself, but do you
know if  slot-alter errors out for other invalidation causes as well?
Just wanted to confirm that the behaviour is consistent for all
invalidation causes.

2)
When a slot is invalidated, and we try to use that slot, it gives this msg:

ERROR:  can no longer get changes from replication slot "mysubnew1_2"
DETAIL:  The slot became invalid because it was inactive since
2024-09-03 14:23:34.094067+05:30, which is more than 600 seconds ago.
HINT:  You might need to increase "replication_slot_inactive_timeout.".

Isn't HINT misleading? Even if we increase it now, the slot can not be
reused again.

3)
When the slot is invalidated, the' inactive_since' still keeps on
changing when there is a subscriber trying to start replication
continuously. I think ReplicationSlotAcquire() keeps on failing and
thus Release keeps on setting it again and again. Shouldn't we stop
setting/chnaging  'inactive_since' once the slot is invalidated
already, otherwise it will be misleading.

postgres=# select failover,synced,inactive_since,invalidation_reason
from pg_replication_slots;

 failover | synced |          inactive_since          | invalidation_reason
----------+--------+----------------------------------+---------------------
 t        | f      | 2024-09-03 14:23:.. | inactive_timeout

after sometime:
 failover | synced |          inactive_since          | invalidation_reason
----------+--------+----------------------------------+---------------------
 t        | f      | 2024-09-03 14:26:..| inactive_timeout

4)
src/sgml/config.sgml:

4a)
+ A value of zero (which is default) disables the timeout mechanism.

Better will be:
A value of zero (which is default) disables the inactive timeout
invalidation mechanism .
or
A value of zero (which is default) disables the slot invalidation due
to the inactive timeout mechanism.

i.e. rephrase to indicate that invalidation is disabled.

4b)
'synced' and inactive_since should point to pg_replication_slots:

example:
<link linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>synced</structfield>

5)
src/sgml/system-views.sgml:
+ ..the slot has been inactive for longer than the duration specified
by replication_slot_inactive_timeout parameter.

Better to have:
..the slot has been inactive for a time longer than the duration
specified by the replication_slot_inactive_timeout parameter.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Wed, 4 Sep 2024 09:17:33 +0530

> Just wanted to confirm that the behaviour is consistent for all

I was able to test this and as anticipated behavior is different. When
slot is invalidated due to say 'wal_removed', I am still able to do
'alter' of that slot.
Please see:

Pub:
  slot_name  | failover | synced |          inactive_since          |
invalidation_reason
-------------+----------+--------+----------------------------------+---------------------
 mysubnew1_1 | t        | f      | 2024-09-04 08:58:12.802278+05:30 |
wal_removed

Sub:
newdb1=# alter subscription mysubnew1_1 disable;
ALTER SUBSCRIPTION

newdb1=# alter subscription mysubnew1_1 set (failover=false);
ALTER SUBSCRIPTION

Pub: (failover altered)
  slot_name  | failover | synced |          inactive_since          |
invalidation_reason
-------------+----------+--------+----------------------------------+---------------------
 mysubnew1_1 | f        | f      | 2024-09-04 08:58:47.824471+05:30 |
wal_removed

while when invalidation_reason is 'inactive_timeout', it fails:

Pub:
  slot_name  | failover | synced |          inactive_since          |
invalidation_reason
-------------+----------+--------+----------------------------------+---------------------
 mysubnew1_1 | t        | f      | 2024-09-03 14:30:57.532206+05:30 |
inactive_timeout

Sub:
newdb1=# alter subscription mysubnew1_1 disable;
ALTER SUBSCRIPTION

newdb1=# alter subscription mysubnew1_1 set (failover=false);
ERROR:  could not alter replication slot "mysubnew1_1": ERROR:  can no
longer get changes from replication slot "mysubnew1_1"
DETAIL:  The slot became invalid because it was inactive since
2024-09-04 08:54:20.308996+05:30, which is more than 0 seconds ago.
HINT:  You might need to increase "replication_slot_inactive_timeout.".

I think the behavior should be same.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Wed, 4 Sep 2024 14:48:51 +0530

> On Tue, Sep 3, 2024 at 3:01 PM shveta malik <[email-masked]> wrote:

1)
It is related to one of my previous comments (pt 3 in [1]) where I
stated that inactive_since should not keep on changing once a slot is
invalidated.
Below is one side effect if inactive_since keeps on changing:

postgres=# SELECT * FROM pg_replication_slot_advance('mysubnew1_1',
pg_current_wal_lsn());
ERROR:  can no longer get changes from replication slot "mysubnew1_1"
DETAIL:  The slot became invalid because it was inactive since
2024-09-04 10:03:56.68053+05:30, which is more than 10 seconds ago.
HINT:  You might need to increase "replication_slot_inactive_timeout.".

postgres=# select now();
               now
---------------------------------
 2024-09-04 10:04:00.26564+05:30

'DETAIL' gives wrong information, we are not past 10-seconds. This is
because inactive_since got updated even in ERROR scenario.

2)
One more issue in this message is, once I set
replication_slot_inactive_timeout to a bigger value, it becomes more
misleading. This is because invalidation was done in the past using
previous value while message starts showing new value:

ALTER SYSTEM SET replication_slot_inactive_timeout TO '36h';

--see 129600 secs in DETAIL and the current time.
postgres=# SELECT * FROM pg_replication_slot_advance('mysubnew1_1',
pg_current_wal_lsn());
ERROR:  can no longer get changes from replication slot "mysubnew1_1"
DETAIL:  The slot became invalid because it was inactive since
2024-09-04 10:06:38.980939+05:30, which is more than 129600 seconds
ago.
postgres=# select now();
               now
----------------------------------
 2024-09-04 10:07:35.201894+05:30

I feel we should change this message itself.

~~~~~

When invalidation is due to wal_removed, we get a way simpler message:

newdb1=# SELECT * FROM pg_replication_slot_advance('mysubnew1_2',
pg_current_wal_lsn());
ERROR:  replication slot "mysubnew1_2" cannot be advanced
DETAIL:  This slot has never previously reserved WAL, or it has been
invalidated.

This message does not mention 'max_slot_wal_keep_size'. We should have
a similar message for our case. Thoughts?

[1]:  https://www.postgresql.org/message-id/CAJpy0uC8Dg-0JS3NRUwVUemgz5Ar2v3_EQQFXyAigWSEQ8U47Q%40mail.gmail.com

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Thu, 5 Sep 2024 09:25:51 +0530

> stated that inactive_since should not keep on changing once a slot is

Agreed. Updating the inactive_since for a slot that is already invalid
is misleading.

> 2024-09-04 10:06:38.980939+05:30, which is more than 129600 seconds

+1.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 5 Sep 2024 09:30:16 +0530

>   slot_name  | failover | synced |          inactive_since          |

We should not allow the invalid replication slot to be altered
irrespective of the reason unless there is any benefit.

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Sun, 8 Sep 2024 17:24:47 +0530

Hi,

Thanks for reviewing.

> please see my doc review comment #4 suggestion below.

+1

+1.

+1.

+1.

+1.

> 'IsSlotATimeoutCandidate()'

Missing inactive in the above suggested name. Used
SlotInactiveTimeoutCheckAllowed, similar to XLogInsertAllowed.

+1.

+1.

+1.

+1.

+1.

+1.

> Please refer to the attached file which implements some of the nits

Merged the diff into v45. Thanks.

> synchronized slot is already invalidated with the correct reason as

Modified the comment.

> Shouldn't your test scenario here be done using a valid synced slot?

+1. Added another test case for checking the synced slot not getting
invalidated despite inactive timeout being set.

> time to happen -- e.g. I doubt 2s has elapsed yet.

Added sleep(timeout+1) before the checkpoint.

Done.

Removed.

Done.

> Why are there are 2 separate poll_query_until's here? Can't those be

Ah. My bad. Removed.

Done.

Modified.

> e.g. "CHECKPOINT_and_wait_for_slot_invalidation_in_server_log"

That would be too long. Changed the function name to
trigger_slot_invalidation() which is appropriate.

Please find the v45 patch. Addressed above and Shveta's review comments [1].

Amit's comments [2] and [3] are still pending.

[1] https://www.postgresql.org/message-id/CAJpy0uC8Dg-0JS3NRUwVUemgz5Ar2v3_EQQFXyAigWSEQ8U47Q%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAA4eK1K7DdT_5HnOWs5tVPYC%3D-h%2Bm85wu7k-7RVJaJ7zMxprWQ%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CAA4eK1%2Bkt-QRr1RP%3DD%3D4_tp%2BS%2BCErQ6rNe7KVYEyZ3f6PYXpvw%40mail.gmail.com

---

From: Bharath Rupireddy <[email-masked]>
Date: Sun, 8 Sep 2024 17:25:42 +0530

Hi,

Thanks for reviewing.

> Just wanted to confirm that the behaviour is consistent for all

Will respond to Amit's comment soon.

> HINT:  You might need to increase "replication_slot_inactive_timeout.".

Removed the hint and corrected the detail message as following:

errmsg("can no longer get changes from replication slot \"%s\"",
NameStr(s->data.name)),
errdetail("This slot has been invalidated because it was inactive for
longer than the amount of time specified by \"%s\".",
"replication_slot_inactive_timeout.")));

>  failover | synced |          inactive_since          | invalidation_reason

Changed it to not update inactive_since for slots invalidated due to
inactive timeout.

Changed.

Modified.

> specified by the replication_slot_inactive_timeout parameter.

Changed it to the following to be consistent with the config.sgml.

          <literal>inactive_timeout</literal> means that the slot has been
          inactive for longer than the amount of time specified by the
          <xref linkend="guc-replication-slot-inactive-timeout"/> parameter.

Please find the v45 patch posted upthread at
https://www.postgresql.org/message-id/CALj2ACWXQT3_HY40ceqKf1DadjLQP6b1r%3D0sZRh-xhAOd-b0pA%40mail.gmail.com
for the changes.

---

From: shveta malik <[email-masked]>
Date: Mon, 9 Sep 2024 09:17:30 +0530

> irrespective of the reason unless there is any benefit.

Okay, then I think we need to change the existing behaviour of the
other invalidation causes which still allow alter-slot.

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 9 Sep 2024 10:26:17 +0530

Hi,

> other invalidation causes which still allow alter-slot.

+1. Perhaps, track it in a separate thread?

---

From: shveta malik <[email-masked]>
Date: Mon, 9 Sep 2024 10:28:42 +0530

> +1. Perhaps, track it in a separate thread?

I think so. It does not come under the scope of this thread.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Mon, 9 Sep 2024 10:53:50 +0530

> Please find the v45 patch. Addressed above and Shveta's review comments [1].

Thanks for the patch. Please find my comments:

1)
src/sgml/config.sgml:

+  Synced slots are always considered to be inactive because they
don't perform logical decoding to produce changes.

It is better we avoid such a statement, as internally we use logical
decoding to advance restart-lsn, see
'LogicalSlotAdvanceAndCheckSnapState' called form slotsync.c.
<Also see related comment 6 below>

2)
src/sgml/config.sgml:

+ disables the inactive timeout invalidation mechanism

Either have 'inactive' at both the places or 'inactivity'.

3)
slot.c:
+static bool InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause
cause,
+    ReplicationSlot *s,
+    XLogRecPtr oldestLSN,
+    Oid dboid,
+    TransactionId snapshotConflictHorizon,
+    bool *invalidated);
+static inline bool SlotInactiveTimeoutCheckAllowed(ReplicationSlot *s);

I think, we do not need above 2 declarations. The code compile fine
without these as the usage is later than the definition.

4)
+ /*

The comment is generic while the 'if condition' is specific to one
invalidation cause. Even though I feel it can be made generic test for
all invalidation causes but that is not under scope of this thread and
needs more testing/analysis. For the time being, we can make comment
specific to the concerned invalidation cause. The header of function
will also need the same change.

5)
SlotInactiveTimeoutCheckAllowed():

+ * Check if inactive timeout invalidation mechanism is disabled or slot is
+ * currently being used or server is in recovery mode or slot on standby is
+ * currently being synced from the primary.
+ *

These comments say exact opposite of what we are checking in code.
Since the function name has 'Allowed' in it, we should be putting
comments which say what allows it instead of what disallows it.

6)

+ * Synced slots are always considered to be inactive because they don't
+ * perform logical decoding to produce changes.
+ */
+static inline bool
+SlotInactiveTimeoutCheckAllowed(ReplicationSlot *s)

Perhaps we should avoid mentioning logical decoding here. When slots
are synced, they are performing decoding and their inactive_since is
changing continuously. A better way to make this statement will be:

We want to ensure that the slots being synchronized are not
invalidated, as they need to be preserved for future use when the
standby server is promoted to the primary. This is necessary for
resuming logical replication from the new primary server.
<Rephrase if needed>

7)

InvalidatePossiblyObsoleteSlot()

we are calling SlotInactiveTimeoutCheckAllowed() twice in this
function. We shall optimize.

At the first usage place, shall we simply get timestamp when cause is
RS_INVAL_INACTIVE_TIMEOUT without checking
SlotInactiveTimeoutCheckAllowed() as IMO it does not seem a
performance critical section. Or if we retain check at first place,
then at the second place we can avoid calling it again based on
whether 'now' is NULL or not.

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Mon, 9 Sep 2024 17:40:50 +1000

Hi, here are some review comments for v45-0001 (excluding the test code)

======
doc/src/sgml/config.sgml

1.
+        Note that the inactive timeout invalidation mechanism is not
+        applicable for slots on the standby server that are being synced
+        from primary server (i.e., standby slots having

nit - /from primary server/from the primary server/

======

2. ReplicationSlotAcquire

+ errmsg("can no longer get changes from replication slot \"%s\"",
+ NameStr(s->data.name)),
+ errdetail("This slot has been invalidated because it was inactive
for longer than the amount of time specified by \"%s\".",
+    "replication_slot_inactive_timeout.")));

nit - "replication_slot_inactive_timeout." - should be no period
inside that GUC name literal

~~~

3. ReportSlotInvalidation

I didn't understand why there was a hint for:
"You might need to increase \"%s\".", "max_slot_wal_keep_size"

But you don't have an equivalent hint for timeout invalidation:
"You might need to increase \"%s\".", "replication_slot_inactive_timeout"

Why aren't these similar cases consistent?

~~~

4. RestoreSlotFromDisk

+ /* Use the same inactive_since time for all the slots. */
+ if (now == 0)
+ now = GetCurrentTimestamp();
+

Is the deferred assignment really necessary? Why not just
unconditionally assign the 'now' just before the for-loop? Or even at
the declaration? e.g. The 'replication_slot_inactive_timeout' is
measured in seconds so I don't think 'inactive_since' being wrong by a
millisecond here will make any difference.

======

5. ReplicationSlotSetInactiveSince

+/*
+ * Set slot's inactive_since property unless it was previously invalidated due
+ * to inactive timeout.
+ */
+static inline void
+ReplicationSlotSetInactiveSince(ReplicationSlot *s, TimestampTz *now,
+ bool acquire_lock)
+{
+ if (acquire_lock)
+ SpinLockAcquire(&s->mutex);
+
+ if (s->data.invalidated != RS_INVAL_INACTIVE_TIMEOUT)
+ s->inactive_since = *now;
+
+ if (acquire_lock)
+ SpinLockRelease(&s->mutex);
+}

Is the logic correct? What if the slot was already invalid due to some
reason other than RS_INVAL_INACTIVE_TIMEOUT? Is an Assert needed?

======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 27b2285..97b4fb5 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4582,7 +4582,7 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        <para>
         Note that the inactive timeout invalidation mechanism is not
         applicable for slots on the standby server that are being synced
-        from primary server (i.e., standby slots having
+        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
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index d92b92b..8cc67b4 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -640,7 +640,7 @@ retry:
 				 errmsg("can no longer get changes from replication slot \"%s\"",
 						NameStr(s->data.name)),
 				 errdetail("This slot has been invalidated because it was inactive for longer than the amount of time specified by \"%s\".",
-						   "replication_slot_inactive_timeout.")));
+						   "replication_slot_inactive_timeout")));
 	}
 
 	/*

---

From: Amit Kapila <[email-masked]>
Date: Mon, 9 Sep 2024 15:04:44 +0530

> I think so. It does not come under the scope of this thread.

It makes sense to me as well. But let's go ahead and get that sorted out first.

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 10 Sep 2024 00:12:50 +0530

Hi,

> It makes sense to me as well. But let's go ahead and get that sorted out first.

Moved the discussion to new thread -
https://www.postgresql.org/message-id/CALj2ACW4fSOMiKjQ3%3D2NVBMTZRTG8Ujg6jsK9z3EvOtvA4vzKQ%40mail.gmail.com.
Please have a look.

---

From: Peter Smith <[email-masked]>
Date: Tue, 10 Sep 2024 11:34:24 +1000

Hi, here is the remainder of my v45-0001 review. These comments are
for the test code only.

======
Testcase #1

1.
+# Testcase start
+#
+# Invalidate streaming standby slot and logical failover slot on primary due to
+# inactive timeout. Also, check logical failover slot synced to standby from
+# primary doesn't invalidate on its own, but gets the invalidated
state from the
+# primary.

nit - s/primary/the primary/ (in a couple of places)
nit - s/standby/the standby/
nit - other trivial tweaks.

~~~

2.
+# Create sync slot on primary
+$primary->psql('postgres',
+ q{SELECT pg_create_logical_replication_slot('sync_slot1',
'test_decoding', false, false, true);}
+);

nit - s/primary/the primary/

~~~

3.
+$primary->safe_psql(
+ 'postgres', qq[
+    SELECT pg_create_physical_replication_slot(slot_name :=
'sb_slot1', immediately_reserve := true);
+]);

Should this have a comment?

~~~

4.
+# Wait until standby has replayed enough data
+$primary->wait_for_catchup($standby1);

nit - s/standby/the standby/

~~~

5.
+# Sync primary slot to standby
+$standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");

nit - /Sync primary slot to standby/Sync the primary slots to the standby/

~~~

6.
+# Confirm that logical failover slot is created on standby

nit - s/Confirm that logical failover slot is created on
standby/Confirm that the logical failover slot is created on the
standby/

~~~

7.
+is( $standby1->safe_psql(
+ 'postgres',
+ q{SELECT count(*) = 1 FROM pg_replication_slots
+   WHERE slot_name = 'sync_slot1' AND synced AND NOT temporary;}
+ ),
+ "t",
+ 'logical slot sync_slot1 has synced as true on standby');

IMO here you should also be checking that the sync slot state is NOT
invalidated, just as a counterpoint for the test part later that
checks that it IS invalidated.

~~~

8.
+my $inactive_timeout = 1;
+
+# Set timeout so that next checkpoint will invalidate inactive slot
+$primary->safe_psql(
+ 'postgres', qq[
+    ALTER SYSTEM SET replication_slot_inactive_timeout TO
'${inactive_timeout}s';
+]);
+$primary->reload;

8a.
nit - I think that $inactive_timeout assignment belongs below your comment.

~

8b.
nit - s/Set timeout so that next checkpoint will invalidate inactive
slot/Set timeout GUC so that the next checkpoint will invalidate
inactive slots/

~~~

9.
+# Check for logical failover slot to become inactive on primary. Note that
+# nobody has acquired slot yet, so it must get invalidated due to
+# inactive timeout.

nit - /Check for logical failover slot to become inactive on
primary./Wait for logical failover slot to become inactive on the
primary./
nit - /has acquired slot/has acquired the slot/

~~~

10.
+# Sync primary slot to standby. Note that primary slot has already been
+# invalidated due to inactive timeout. Standby must just sync inavalidated
+# state.

nit - minor, add "the". fix typo "inavalidated", etc. suggestion:

Re-sync the primary slots to the standby. Note that the primary slot was already
invalidated (above) due to inactive timeout. The standby must just
sync the invalidated
state.

~~~

11.
+# Make standby slot on primary inactive and check for invalidation
+$standby1->stop;

nit - /standby slot/the standby slot/
nit - /on primary/on the primary/

======
Testcase #2

12.
I'm not sure it is necessary to do all this extra work. IIUC, there
was already almost everything you needed in the previous Testcase #1.
So, I thought you could just combine this extra standby timeout test
in Testcase #1.

Indeed, your Testcase #1 comment still says it is doing this: ("Also,
check logical failover slot synced to standby from primary doesn't
invalidate on its own,...")

e.g.
- NEW: set the GUC timeout on the standby
- sync the sync_slot (already doing in test #1)
- ensure the synced slot is NOT invalid (already suggested above for test #1)
- NEW: then do a standby sleep > timeout duration
- NEW: then do a standby CHECKPOINT...
- NEW: then ensure the sync slot invalidation did NOT happen
- then proceed with the rest of test #1...

======
Testcase #3

13.
nit - remove a few blank lines to group associated statements together.

~~~

14.
+$publisher->safe_psql(
+ 'postgres', qq[
+    ALTER SYSTEM SET replication_slot_inactive_timeout TO '
${inactive_timeout}s';
+]);
+$publisher->reload;

nit - this deserves a comment, the same as in Testcase #1

======

15.
+# Check for slot to first become inactive and then get invalidated
+sub check_for_slot_invalidation

nit - IMO the previous name was better (e.g. "wait_for.." instead of
"check_for...") because that describes exactly what the subroutine is
doing.

suggestion:
# Wait for the slot to first become inactive and then get invalidated
sub wait_for_slot_invalidation

~~~

16.
+{
+ my ($node, $slot, $offset, $inactive_timeout) = @_;
+ my $name = $node->name;

The variable $name seems too vague. How about $node_name?

~~~

17.
+ # Wait for invalidation reason to be set
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+ WHERE slot_name = '$slot' AND
+ invalidation_reason = 'inactive_timeout';
+ ])
+   or die
+   "Timed out while waiting for invalidation reason of slot $slot to
be set on node $name";

17a.
nit - /# Wait for invalidation reason to be set/# Check that the
invalidation reason is 'inactive_timeout'/

IIUC, the 'trigger_slot_invalidation' function has already invalidated
the slot at this point, so we are not really "Waiting..."; we are
"Checking..." that the reason was correctly set.

~

17b.
I think this code fragment maybe would be better put inside the
'trigger_slot_invalidation' function. (I've done this in the nitpicks
attachment)

~~~

18.
+ # Check that invalidated slot cannot be acquired
+ my ($result, $stdout, $stderr);
+
+ ($result, $stdout, $stderr) = $node->psql(
+ 'postgres', qq[
+ SELECT pg_replication_slot_advance('$slot', '0/1');
+ ]);

18a.
s/Check that invalidated slot/Check that an invalidated slot/

~

18b.
nit - Remove some blank lines, because the comment applies to all below it.

======
sub trigger_slot_invalidation

19.
+# Trigger slot invalidation and confirm it in server log
+sub trigger_slot_invalidation

nit - s/confirm it in server log/confirm it in the server log/

~

20.
+{
+ my ($node, $slot, $offset, $inactive_timeout) = @_;
+ my $name = $node->name;
+ my $invalidated = 0;

(same as the other subroutine)
nit - The variable $name seems too vague. How about $node_name?

======

Please refer to the attached nitpicks top-up patch which implements
most of the above nits.

======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl
index 669d6cc..34b46d5 100644
--- a/src/test/recovery/t/050_invalidate_slots.pl
+++ b/src/test/recovery/t/050_invalidate_slots.pl
@@ -12,10 +12,10 @@ use Time::HiRes qw(usleep);
 # =============================================================================
 # Testcase start
 #
-# Invalidate streaming standby slot and logical failover slot on primary due to
-# inactive timeout. Also, check logical failover slot synced to standby from
-# primary doesn't invalidate on its own, but gets the invalidated state from the
-# primary.
+# Invalidate a streaming standby slot and logical failover slot on the primary
+# due to inactive timeout. Also, check that a logical failover slot synced to
+# the standby from the primary doesn't invalidate on its own, but gets the
+# invalidated state from the primary.
 
 # Initialize primary
 my $primary = PostgreSQL::Test::Cluster->new('primary');
@@ -45,7 +45,7 @@ primary_slot_name = 'sb_slot1'
 primary_conninfo = '$connstr dbname=postgres'
 ));
 
-# Create sync slot on primary
+# Create sync slot on the primary
 $primary->psql('postgres',
 	q{SELECT pg_create_logical_replication_slot('sync_slot1', 'test_decoding', false, false, true);}
 );
@@ -57,13 +57,13 @@ $primary->safe_psql(
 
 $standby1->start;
 
-# Wait until standby has replayed enough data
+# Wait until the standby has replayed enough data
 $primary->wait_for_catchup($standby1);
 
-# Sync primary slot to standby
+# Sync the primary slots to the standby
 $standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
 
-# Confirm that logical failover slot is created on standby
+# Confirm that the logical failover slot is created on the standby
 is( $standby1->safe_psql(
 		'postgres',
 		q{SELECT count(*) = 1 FROM pg_replication_slots
@@ -73,24 +73,24 @@ is( $standby1->safe_psql(
 	'logical slot sync_slot1 has synced as true on standby');
 
 my $logstart = -s $primary->logfile;
-my $inactive_timeout = 1;
 
-# Set timeout so that next checkpoint will invalidate inactive slot
+# Set timeout GUC so that that next checkpoint will invalidate inactive slots
+my $inactive_timeout = 1;
 $primary->safe_psql(
 	'postgres', qq[
     ALTER SYSTEM SET replication_slot_inactive_timeout TO '${inactive_timeout}s';
 ]);
 $primary->reload;
 
-# Check for logical failover slot to become inactive on primary. Note that
+# Wait for logical failover slot to become inactive on the primary. Note that
 # nobody has acquired slot yet, so it must get invalidated due to
 # inactive timeout.
-check_for_slot_invalidation($primary, 'sync_slot1', $logstart,
+wait_for_slot_invalidation($primary, 'sync_slot1', $logstart,
 	$inactive_timeout);
 
-# Sync primary slot to standby. Note that primary slot has already been
-# invalidated due to inactive timeout. Standby must just sync inavalidated
-# state.
+# Re-sync the primary slots to the standby. Note that the primary slot was
+# already invalidated (above) due to inactive timeout. The standby must just
+# sync the invalidated state.
 $standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
 $standby1->poll_query_until(
 	'postgres', qq[
@@ -101,9 +101,9 @@ $standby1->poll_query_until(
   or die
   "Timed out while waiting for sync_slot1 invalidation to be synced on standby";
 
-# Make standby slot on primary inactive and check for invalidation
+# Make the standby slot on the primary inactive and check for invalidation
 $standby1->stop;
-check_for_slot_invalidation($primary, 'sb_slot1', $logstart,
+wait_for_slot_invalidation($primary, 'sb_slot1', $logstart,
 	$inactive_timeout);
 
 # Testcase end
@@ -223,14 +223,12 @@ $publisher->safe_psql(
 $subscriber->safe_psql('postgres',
 	"CREATE SUBSCRIPTION sub CONNECTION '$publisher_connstr' PUBLICATION pub WITH (slot_name = 'lsub1_slot', create_slot = false)"
 );
-
 $subscriber->wait_for_subscription_sync($publisher, 'sub');
-
 my $result =
   $subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tbl");
-
 is($result, qq(5), "check initial copy was done");
 
+# Set timeout GUC so that that next checkpoint will invalidate inactive slots
 $publisher->safe_psql(
 	'postgres', qq[
     ALTER SYSTEM SET replication_slot_inactive_timeout TO ' ${inactive_timeout}s';
@@ -241,17 +239,17 @@ $logstart = -s $publisher->logfile;
 
 # Make subscriber slot on publisher inactive and check for invalidation
 $subscriber->stop;
-check_for_slot_invalidation($publisher, 'lsub1_slot', $logstart,
+wait_for_slot_invalidation($publisher, 'lsub1_slot', $logstart,
 	$inactive_timeout);
 
 # Testcase end
 # =============================================================================
 
-# Check for slot to first become inactive and then get invalidated
-sub check_for_slot_invalidation
+# Wait for slot to first become inactive and then get invalidated
+sub wait_for_slot_invalidation
 {
 	my ($node, $slot, $offset, $inactive_timeout) = @_;
-	my $name = $node->name;
+	my $node_name = $node->name;
 
 	# Wait for slot to become inactive
 	$node->poll_query_until(
@@ -261,45 +259,33 @@ sub check_for_slot_invalidation
 				  inactive_since IS NOT NULL;
 	])
 	  or die
-	  "Timed out while waiting for slot $slot to become inactive on node $name";
+	  "Timed out while waiting for slot $slot to become inactive on node $node_name";
 
 	trigger_slot_invalidation($node, $slot, $offset, $inactive_timeout);
 
-	# Wait for invalidation reason to be set
-	$node->poll_query_until(
-		'postgres', qq[
-		SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
-			WHERE slot_name = '$slot' AND
-			invalidation_reason = 'inactive_timeout';
-	])
-	  or die
-	  "Timed out while waiting for invalidation reason of slot $slot to be set on node $name";
-
-	# Check that invalidated slot cannot be acquired
+	# 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 get changes from replication slot "$slot"/,
-		"detected error upon trying to acquire invalidated slot $slot on node $name"
+		"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 $name";
+	  "could not detect error upon trying to acquire invalidated slot $slot on node $node_name";
 }
 
-# Trigger slot invalidation and confirm it in server log
+# Trigger slot invalidation and confirm it in the server log
 sub trigger_slot_invalidation
 {
 	my ($node, $slot, $offset, $inactive_timeout) = @_;
-	my $name = $node->name;
+	my $node_name = $node->name;
 	my $invalidated = 0;
 
 	# Give enough time to avoid multiple checkpoints
-	sleep($inactive_timeout+1);
+	sleep($inactive_timeout + 1);
 
 	for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
 	{
@@ -314,8 +300,18 @@ sub trigger_slot_invalidation
 		usleep(100_000);
 	}
 	ok($invalidated,
-		"check that slot $slot invalidation has been logged on node $name"
+		"check that slot $slot invalidation has been logged on node $node_name"
 	);
+
+	# Check that the invalidation reason is 'inactive_timeout'
+	$node->poll_query_until(
+		'postgres', qq[
+		SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+			WHERE slot_name = '$slot' AND
+			invalidation_reason = 'inactive_timeout';
+	])
+	  or die
+	  "Timed out while waiting for invalidation reason of slot $slot to be set on node $node_name";
 }
 
 done_testing();

---

From: Amit Kapila <[email-masked]>
Date: Mon, 16 Sep 2024 08:55:10 +0530

> https://www.postgresql.org/message-id/CALj2ACW4fSOMiKjQ3%3D2NVBMTZRTG8Ujg6jsK9z3EvOtvA4vzKQ%40mail.gmail.com.

That is pushed now. Please send the rebased patch after addressing the
pending comments.

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 16 Sep 2024 15:17:47 +0530

Hi,

Thanks for reviewing.

> Either have 'inactive' at both the places or 'inactivity'.

Used "inactive timeout".

> without these as the usage is later than the definition.

Hm, it's a usual practice that I follow irrespective of the placement
of function declarations. Since it was brought up, I removed the
declarations.

> needs more testing/analysis.

Right.

> For the time being, we can make comment

Adjusted the comment, but left the variable name error_if_invalid as
is. Didn't want to make it long, one can look at the code to
understand what it is used for.

Modified.

> resuming logical replication from the new primary server.

They are performing logical decoding, but not producing the changes
for the clients to consume. So, IMO, the accompanying "to produce
changes" next to the "logical decoding" is good here.

> whether 'now' is NULL or not.

Getting a current timestamp can get costlier on platforms that use
various clock sources, so assigning 'now' unconditionally isn't the
way IMO. Using the inline function in two places improves the
readability. Can optimize it if there's any performance impact of
calling the inline function in two places.

Will post the new patch version soon.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 16 Sep 2024 15:31:11 +0530

Hi,

Thanks for reviewing.

+1

> inside that GUC name literal

Typo. Fixed.

> Why aren't these similar cases consistent?

It looks misleading and not very useful. What happens if the removed
WAL (that's needed for the slot) is put back into pg_wal somehow (by
manually copying from archive or by some tool/script)? Can the slot
invalidated due to wal_removed start sending WAL to its clients?

> "You might need to increase \"%s\".", "replication_slot_inactive_timeout"

I removed this per review comments upthread.

> millisecond here will make any difference.

Moved it before the for-loop.

> reason other than RS_INVAL_INACTIVE_TIMEOUT? Is an Assert needed?

Hm. Since invalidated slots can't be acquired and made active, not
modifying inactive_since irrespective of invalidation reason looks
good to me.

Please find the attached v46 patch having changes for the above review
comments and your test review comments and Shveta's review comments.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 16 Sep 2024 16:54:40 +0530

-ReplicationSlotAcquire(const char *name, bool nowait)
+ReplicationSlotAcquire(const char *name, bool nowait, bool error_if_invalid)
 {
  ReplicationSlot *s;
  int active_pid;
@@ -615,6 +620,22 @@ retry:
  /* We made this slot active, so it's ours now. */
  MyReplicationSlot = s;

+ /*
+ * 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_INACTIVE_TIMEOUT)
+ {
+ Assert(s->inactive_since > 0);
+ 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 was inactive
for longer than the amount of time specified by \"%s\".",
+    "replication_slot_inactive_timeout")));
+ }

Why raise the ERROR just for timeout invalidation here and why not if
the slot is invalidated for other reasons? This raises the question of
what happens before this patch if the invalid slot is used from places
where we call ReplicationSlotAcquire(). I did a brief code analysis
and found that for StartLogicalReplication(), even if the error won't
occur in ReplicationSlotAcquire(), it would have been caught in
CreateDecodingContext(). I think that is where we should also add this
new error. Similarly, pg_logical_slot_get_changes_guts() and other
logical replication functions should be calling
CreateDecodingContext() which can raise the new ERROR. I am not sure
about how the invalid slots are handled during physical replication,
please check the behavior of that before this patch.

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 16 Sep 2024 22:40:52 +0530

Hi,

> please check the behavior of that before this patch.

When physical slots are invalidated due to wal_removed reason, the failure
happens at a much later point for the streaming standbys while reading the
requested WAL files like the following:

2024-09-16 16:29:52.416 UTC [876059] FATAL:  could not receive data from
WAL stream: ERROR:  requested WAL segment 000000010000000000000005 has
already been removed
2024-09-16 16:29:52.416 UTC [872418] LOG:  waiting for WAL to become
available at 0/5002000

At this point, despite the slot being invalidated, its wal_status can still
come back to 'unreserved' even from 'lost', and the standby can catch up if
removed WAL files are copied either by manually or by a tool/script to the
primary's pg_wal directory. IOW, the physical slots invalidated due to
wal_removed are *somehow* recoverable unlike the logical slots.

IIUC, the invalidation of a slot implies that it is not guaranteed to hold
any resources like WAL and XMINs. Does it also imply that the slot must be
unusable?

---

From: Peter Smith <[email-masked]>
Date: Tue, 17 Sep 2024 11:27:24 +1000

Here are a few comments for the patch v46-0001.

======

1. ReportSlotInvalidation

> I removed this per review comments upthread.

IIUC the errors are quite similar, so my previous review comment was
mostly about the unexpected inconsistency of why one of them has a
hint and the other one does not. I don't have a strong opinion about
whether they should both *have* or *not have* hints, so long as they
are treated the same.

If you think the current code hint is not useful then maybe we need a
new thread to address that existing issue. For example, maybe it
should be removed or reworded.

~~~

2. InvalidatePossiblyObsoleteSlot:

+ case RS_INVAL_INACTIVE_TIMEOUT:
+
+ if (!SlotInactiveTimeoutCheckAllowed(s))
+ break;
+
+ /*
+ * Check if the slot needs to be invalidated due to
+ * replication_slot_inactive_timeout GUC.
+ */
+ if (TimestampDifferenceExceeds(s->inactive_since, now,
+    replication_slot_inactive_timeout * 1000))

nit - it might be tidier to avoid multiple breaks by just combining
these conditions. See the nitpick attachment.

~~~

3.
  * - RS_INVAL_WAL_LEVEL: is logical
+ * - RS_INVAL_INACTIVE_TIMEOUT: inactive timeout occurs

nit - use comment wording "inactive slot timeout has occurred", to
make it identical to the comment in slot.h

======

4.
+# Despite inactive timeout being set, the synced slot won't get invalidated on
+# its own on the standby. So, we must not see invalidation message in server
+# log.
+$standby1->safe_psql('postgres', "CHECKPOINT");
+ok( !$standby1->log_contains(
+ "invalidating obsolete replication slot \"sync_slot1\"",
+ $logstart),
+ 'check that synced slot sync_slot1 has not been invalidated on standby'
+);
+

It seems kind of brittle to check the logs for something that is NOT
there because any change to the message will make this accidentally
pass. Apart from that, it might anyway be more efficient just to check
the pg_replication_slots again to make sure the 'invalidation_reason
remains' still NULL.

======

Please see the attachment which implements some of the nit changes
mentioned above.

======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 851120e..0076e4b 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1716,15 +1716,12 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
 						invalidation_cause = cause;
 					break;
 				case RS_INVAL_INACTIVE_TIMEOUT:
-
-					if (!SlotInactiveTimeoutCheckAllowed(s))
-						break;
-
 					/*
 					 * Check if the slot needs to be invalidated due to
 					 * replication_slot_inactive_timeout GUC.
 					 */
-					if (TimestampDifferenceExceeds(s->inactive_since, now,
+					if (SlotInactiveTimeoutCheckAllowed(s) &&
+						TimestampDifferenceExceeds(s->inactive_since, now,
 												   replication_slot_inactive_timeout * 1000))
 					{
 						invalidation_cause = cause;
@@ -1894,7 +1891,7 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
  * - RS_INVAL_HORIZON: requires a snapshot <= the given horizon in the given
  *   db; dboid may be InvalidOid for shared relations
  * - RS_INVAL_WAL_LEVEL: is logical
- * - RS_INVAL_INACTIVE_TIMEOUT: inactive timeout occurs
+ * - RS_INVAL_INACTIVE_TIMEOUT: inactive slot timeout has occurred
  *
  * NB - this runs as part of checkpoint, so avoid raising errors if possible.
  */
diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl
index c53b5b3..e2fdd52 100644
--- a/src/test/recovery/t/050_invalidate_slots.pl
+++ b/src/test/recovery/t/050_invalidate_slots.pl
@@ -87,7 +87,7 @@ is( $standby1->safe_psql(
 	'logical slot sync_slot1 is synced to standby');
 
 # Give enough time
-sleep($inactive_timeout+1);
+sleep($inactive_timeout + 1);
 
 # Despite inactive timeout being set, the synced slot won't get invalidated on
 # its own on the standby. So, we must not see invalidation message in server

---

From: shveta malik <[email-masked]>
Date: Wed, 18 Sep 2024 12:21:56 +0530

> comments and your test review comments and Shveta's review comments.

Thanks for addressing comments.

Is there a reason that we don't support this invalidation on hot
standby for non-synced slots? Shouldn't we support this time-based
invalidation there too just like other invalidations?

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Wed, 18 Sep 2024 14:49:00 +0530

> invalidation there too just like other invalidations?

Now since we are not changing inactive_since once it is invalidated,
we are not even initializing it during restart; and thus later when
someone tries to use slot, it leads to assert in
ReplicationSlotAcquire()  ( Assert(s->inactive_since > 0);

Steps:
--Disable logical subscriber and let the slot on publisher gets
invalidated due to inactive_timeout.
--Enable the logical subscriber again.
--Restart publisher.

a) We should initialize inactive_since when
ReplicationSlotSetInactiveSince() is called from RestoreSlotFromDisk()
even though it is invalidated.
b) And shall we mention in the doc of 'active_since', that once the
slot is invalidated, this value will remain unchanged until we
shutdown the server. On server restart, it is initialized to start
time. Thought?

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Wed, 18 Sep 2024 15:31:16 +0530

> > > comments and your test review comments and Shveta's review comments.

When the synced slot is marked as 'inactive_timeout' invalidated on
hot standby due to invalidation of publisher 's failover slot, the
former starts showing NULL' inactive_since'. Is this intentional
behaviour? I feel inactive_since should be non-NULL here too?
Thoughts?

physical standby:
postgres=# select slot_name, inactive_since, invalidation_reason,
failover, synced from pg_replication_slots;
slot_name  |          inactive_since                              |
invalidation_reason | failover | synced
-------------+----------------------------------+---------------------+----------+--------
sub2 | 2024-09-18 15:20:04.364998+05:30 |           | t        | t
sub3 | 2024-09-18 15:20:04.364953+05:30 |           | t        | t

After sync of invalidation_reason:

slot_name  |          inactive_since          | invalidation_reason |
failover | synced
-------------+----------------------------------+---------------------+----------+--------
 sub2 |                               | inactive_timeout    | t        | t
 sub3 |                               | inactive_timeout    | t        | t

thanks
shveta

---

From: Amit Kapila <[email-masked]>
Date: Wed, 18 Sep 2024 17:40:10 +0530

On Mon, Sep 16, 2024 at 10:41 PM Bharath Rupireddy
<[email-masked]> wrote:
> When physical slots are invalidated due to wal_removed reason, the failure happens at a much later point for the streaming standbys while reading the requested WAL files like the following:
>
> 2024-09-16 16:29:52.416 UTC [876059] FATAL:  could not receive data from WAL stream: ERROR:  requested WAL segment 000000010000000000000005 has already been removed
> 2024-09-16 16:29:52.416 UTC [872418] LOG:  waiting for WAL to become available at 0/5002000
>
> At this point, despite the slot being invalidated, its wal_status can still come back to 'unreserved' even from 'lost', and the standby can catch up if removed WAL files are copied either by manually or by a tool/script to the primary's pg_wal directory. IOW, the physical slots invalidated due to wal_removed are *somehow* recoverable unlike the logical slots.
>
> IIUC, the invalidation of a slot implies that it is not guaranteed to hold any resources like WAL and XMINs. Does it also imply that the slot must be unusable?
>

If we can't hold the dead rows against xmin of the invalid slot, then
how can we make it usable even after copying the required WAL?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Thu, 19 Sep 2024 09:40:12 +0530

> > > > comments and your test review comments and Shveta's review comments.

When we promote hot standby with synced logical slots to become new
primary, the logical slots are never invalidated with
'inactive_timeout' on new primary.  It seems the check in
SlotInactiveTimeoutCheckAllowed() is wrong. We should allow
invalidation of slots on primary even if they are marked as 'synced'.
Please see [4].
I have raised 4 issues so far on v46, the first 3 are in [1],[2],[3].
Once all these are addressed, I can continue reviewing further.

[1]: https://www.postgresql.org/message-id/CAJpy0uAwxc49Dz6t%3D-y_-z-MU%2BA4RWX4BR3Zri_jj2qgGMq_8g%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAJpy0uC6nN3SLbEuCvz7-CpaPdNdXxH%3DfeW5MhYQch-JWV0tLg%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAJpy0uBXXJC6f04%2BFU1axKaU%2Bp78wN0SEhUNE9XoqbjXj%3Dhhgw%40mail.gmail.com

[4]:
--------------------
postgres=#  select pg_is_in_recovery();
--------
 f

postgres=# show replication_slot_inactive_timeout;
 replication_slot_inactive_timeout
-----------------------------------
 10s

postgres=# select slot_name, inactive_since, invalidation_reason,
synced from pg_replication_slots;
  slot_name  |          inactive_since          | invalidation_reason | synced
-------------+----------------------------------+---------------------+----------+--------
 mysubnew1_1 | 2024-09-19 09:04:09.714283+05:30 |                     | t

postgres=# select now();
               now
----------------------------------
 2024-09-19 09:06:28.871354+05:30

postgres=# checkpoint;
CHECKPOINT

postgres=# select slot_name, inactive_since, invalidation_reason,
synced from pg_replication_slots;
  slot_name  |          inactive_since          | invalidation_reason | synced
-------------+----------------------------------+---------------------+----------+--------
 mysubnew1_1 | 2024-09-19 09:04:09.714283+05:30 |                     | t
--------------------

thanks
Shveta

---

From: Nisha Moond <[email-masked]>
Date: Thu, 7 Nov 2024 15:33:33 +0530

Hi,

I’ve reviewed this thread and am interested in working on the
remaining tasks and comments, as well as the future review comments.
However, Bharath, please let me know if you'd prefer to continue with
it.

Attached the rebased v47 patch, which also addresses Peter’s comments
#2, #3, and #4 at [1]. I will try addressing other comments as well in
next versions.

[1] https://www.postgresql.org/message-id/CAHut%2BPs%3Dx%2B2Hq5ue0YppOeDZqgHTnyw%3Du%2Bvs-qy0JRjKaeJtew%40mail.gmail.com

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Mon, 11 Nov 2024 18:12:28 +0530

> invalidation_reason | failover | synced

For synced slots on the standby, inactive_since indicates the last
synchronization time rather than the time the slot became inactive
(see doc - https://www.postgresql.org/docs/devel/view-pg-replication-slots.html).

In the reported case above, once a synced slot is invalidated we don't
even keep the last synchronization time for it. This is because when a
synced slot on the standby is marked invalid, inactive_since is reset
to NULL each time the slot-sync worker acquires a lock on it. This
lock acquisition before checking invalidation is done to avoid certain
race conditions and will activate the slot temporarily, resetting
inactive_since. Later, the slot-sync worker updates inactive_since for
all synced slots to the current synchronization time. However, for
invalid slots, this update is skipped, as per the patch’s design.

If we want to preserve the inactive_since value for the invalid synced
slots on standby, we need to clarify the time it should display. Here
are three possible approaches:

1) Copy the primary's inactive_since upon invalidation: When a slot
becomes invalid on the primary, the slot-sync worker could copy the
primary slot’s inactive_since to the standby slot and retain it, by
preventing future updates on the standby.

2) Use the current time of standby when the synced slot is marked
invalid for the first time and do not update it in subsequent sync
cycles if the slot is invalid.

Approach (2) seems more reasonable to me, however, Both 1) & 2)
approaches contradicts the purpose of inactive_since, as it no longer
represents either the true "last sync time" or the "time slot became
inactive" because the slot-sync worker acquires locks periodically for
syncing, and keeps activating the slot.

3) Continuously update inactive_since for invalid synced slots as
well: Treat invalid synced slots like valid ones by updating
inactive_since with each sync cycle. This way, we can keep the "last
sync time" in the inactive_since. However, this could confuse users
when "invalidation_reason=inactive_timeout" is set for a synced slot
on standby but inactive_since would reflect sync time rather than the
time slot became inactive. IIUC, on the primary, when
invalidation_reason=inactive_timeout for a slot, the inactive_since
represents the actual time the slot became inactive before getting
invalidated, unless the primary is restarted.

Thoughts?

--
Thanks,
Nisha

---

From: vignesh C <[email-masked]>
Date: Wed, 13 Nov 2024 14:59:25 +0530

> #2, #3, and #4 at [1]. I will try addressing other comments as well in

The following crash occurs while upgrading:
2024-11-13 14:19:45.955 IST [44539] LOG:  checkpoint starting: time
TRAP: failed Assert("!(*invalidated && SlotIsLogical(s) &&
IsBinaryUpgrade)"), File: "slot.c", Line: 1793, PID: 44539
postgres: checkpointer (ExceptionalCondition+0xbb)[0x555555e305bd]
postgres: checkpointer (+0x63ab04)[0x555555b8eb04]
postgres: checkpointer
(InvalidateObsoleteReplicationSlots+0x149)[0x555555b8ee5f]
postgres: checkpointer (CheckPointReplicationSlots+0x267)[0x555555b8f125]
postgres: checkpointer (+0x1f3ee8)[0x555555747ee8]
postgres: checkpointer (CreateCheckPoint+0x78f)[0x5555557475ee]
postgres: checkpointer (CheckpointerMain+0x632)[0x555555b2f1e7]
postgres: checkpointer (postmaster_child_launch+0x119)[0x555555b30892]
postgres: checkpointer (+0x5e2dc8)[0x555555b36dc8]
postgres: checkpointer (PostmasterMain+0x14bd)[0x555555b33647]
postgres: checkpointer (+0x487f2e)[0x5555559dbf2e]
/lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x7ffff6c29d90]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x7ffff6c29e40]
postgres: checkpointer (_start+0x25)[0x555555634c25]
2024-11-13 14:19:45.967 IST [44538] LOG:  checkpointer process (PID
44539) was terminated by signal 6: Aborted

This can happen in the following case:
1) Setup a logical replication cluster with enough data so that it
will take at least few minutes to upgrade
2) Stop the publisher node
3) Configure replication_slot_inactive_timeout and checkpoint_timeout
to 30 seconds
4) Upgrade the publisher node.

This is happening because logical replication slots are getting
invalidated during upgrade and there is an assertion which checks that
the slots are not invalidated.
I feel this can be fixed by having a function similar to
check_max_slot_wal_keep_size which will make sure that
replication_slot_inactive_timeout is 0 during upgrade.

Regards,
Vignesh

---

From: Nisha Moond <[email-masked]>
Date: Wed, 13 Nov 2024 15:00:28 +0530

Please find the v48 patch attached.

> invalidation of slots on primary even if they are marked as 'synced'.

fixed.

> Once all these are addressed, I can continue reviewing further.

Fixed issues reported in [1], [2].

---

From: Nisha Moond <[email-masked]>
Date: Wed, 13 Nov 2024 15:05:01 +0530

> invalidation there too just like other invalidations?

I don’t see any reason to *not* support this invalidation on hot
standby for non-synced slots. Therefore, I’ve added the same in v48.

--
Thanks,
Nisha

---

From: Peter Smith <[email-masked]>
Date: Thu, 14 Nov 2024 10:58:45 +1100

Hi Nisha.

Thanks for the recent patch updates. Here are my review comments for
the latest patch v48-0001.

======
Commit message

1.
Till now, postgres has the ability to invalidate inactive
replication slots based on the amount of WAL (set via
max_slot_wal_keep_size GUC) that will be needed for the slots in
case they become active. However, choosing a default value for
this GUC is a bit tricky. Because the amount of WAL a database
generates, and the allocated storage for instance will vary
greatly in production, making it difficult to pin down a
one-size-fits-all value.

~

What do the words "for instance" mean here? Did it mean "per instance"
or "(for example)" or something else?

======

2.
       <para>
         The time since the slot has become inactive.
-        <literal>NULL</literal> if the slot is currently being used.
-        Note that for slots on the standby that are being synced from a
+        <literal>NULL</literal> if the slot is currently being used. Once the
+        slot is invalidated, this value will remain unchanged until we shutdown
+        the server. Note that for slots on the standby that are being
synced from a
         primary server (whose <structfield>synced</structfield> field is
         <literal>true</literal>), the

Is this change related to the new inactivity timeout feature or are
you just clarifying the existing behaviour of the 'active_since'
field.

Note there is already another thread [1] created to patch/clarify this
same field. So if you are just clarifying existing behavior then IMO
it would be better if you can to try and get your desired changes
included there quickly before that other patch gets pushed.

~~~

3.
+         <para>
+          <literal>inactive_timeout</literal> means that the slot has been
+          inactive for longer than the amount of time specified by the
+          <xref linkend="guc-replication-slot-inactive-timeout"/> parameter.
+         </para>

Maybe there is a slightly shorter/simpler way to express this. For example,

BEFORE
inactive_timeout means that the slot has been inactive for longer than
the amount of time specified by the replication_slot_inactive_timeout
parameter.

SUGGESTION
inactive_timeout means that the slot has remained inactive beyond the
duration specified by the replication_slot_inactive_timeout parameter.

======

4.
+int replication_slot_inactive_timeout = 0;

IMO it would be more informative to give the units in the variable
name (but not in the GUC name). e.g.
'replication_slot_inactive_timeout_secs'.

~~~

ReplicationSlotAcquire:

5.
+ *

This function comment makes it seem like "invalidated previously"
might mean *any* kind of invalidation, but later in the body of the
function we find the logic is really only used for inactive timeout.

+ /*

So, I think a better name for that parameter might be
'error_if_inactive_timeout'

OTOH, if it really is supposed to erro for *any* kind of invalidation
then there needs to be more ereports.

~~~

6.

This errdetail message seems quite long. I think it can be shortened
like below and still retain exactly the same meaning:

BEFORE:
This slot has been invalidated because it was inactive for longer than
the amount of time specified by \"%s\".

SUGGESTION:
This slot has been invalidated due to inactivity exceeding the time
limit set by "%s".

~~~

ReportSlotInvalidation:

7.
+ case RS_INVAL_INACTIVE_TIMEOUT:
+ Assert(inactive_since > 0);
+ appendStringInfo(&err_detail,
+ _("The slot has been inactive since %s for longer than the amount of
time specified by \"%s\"."),
+ timestamptz_to_str(inactive_since),
+ "replication_slot_inactive_timeout");
+ break;

Here also as in the above review comment #6 I think the message can be
shorter and still say the same thing

BEFORE:
_("The slot has been inactive since %s for longer than the amount of
time specified by \"%s\"."),

SUGGESTION:
_("The slot has been inactive since %s, exceeding the time limit set
by \"%s\"."),

~~~

SlotInactiveTimeoutCheckAllowed:

8.
+/*
+ * Is this replication slot allowed for inactive timeout invalidation check?
+ *
+ * Inactive timeout invalidation is allowed only when:
+ *
+ * 1. Inactive timeout is set
+ * 2. Slot is inactive
+ * 3. Server is in recovery and slot is not being synced from the primary
+ *
+ * Note that the inactive 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.
+ */

8a.
Somehow that first sentence seems strange. Would it be better to write it like:

SUGGESTION
Can this replication slot timeout due to inactivity?

~

8b.
AFAICT that reason 3 ("Server is in recovery and slot is not being
synced from the primary") seems not quite worded right...

Should it say more like:
The slot is not being synced from the primary while the server is in recovery

or maybe like:
The slot is not currently being synced from the primary (e.g. not
'synced' is true when server is in recovery)

~

8c.
Similarly, I think something about that "Note that the inactive
timeout invalidation mechanism is not applicable..." paragraph needs
tweaking because IMO that should also now be saying something about
'RecoveryInProgress'.

~~~

9.
+static inline bool

Maybe the function name should be 'IsSlotInactiveTimeoutPossible' or
something better.

~~~

InvalidatePossiblyObsoleteSlot:

10.
  break;

Since there are no other blank lines anywhere in this switch, the
introduction of this one in v48 looks out of place to me. IMO it would
be more readable if a blank line followed each/every of the breaks,
but then that is not a necessary change for this patch so...

~~~

11.
+ /*
+ * Invalidation due to inactive timeout implies that
+ * no one is using the slot.
+ */
+ Assert(s->active_pid == 0);

Given this assertion, does it mean that "(s->active_pid == 0)" should
have been another condition done up-front in the function
'SlotInactiveTimeoutCheckAllowed'?

~~~

12.
  /*
- * If the slot can be acquired, do so and mark it invalidated
- * immediately.  Otherwise we'll signal the owning process, below, and
- * retry.
+ * If the slot can be acquired, do so and mark it as invalidated. If
+ * the slot is already ours, mark it as invalidated. Otherwise, we'll
+ * signal the owning process below and retry.
  */
- if (active_pid == 0)
+ if (active_pid == 0 ||
+ (MyReplicationSlot == s &&
+ active_pid == MyProcPid))

I wasn't sure how this change belongs to this patch, because the logic
of the previous review comment said for the case of invalidation due
to inactivity that active_id must be 0. e.g. Assert(s->active_pid ==
0);

~~~

RestoreSlotFromDisk:

13.
- slot->inactive_since = GetCurrentTimestamp();
+ slot->inactive_since = now;

In v47 this assignment used to call the function
'ReplicationSlotSetInactiveSince'. I recognise there is a very subtle
difference between direct assignment and the function, because the
function will skip assignment if the slot is already invalidated.
Anyway, if you are *deliberately* not wanting to call
ReplicationSlotSetInactiveSince here then I think this assignment
should be commented to explain the reason why not, otherwise someone
in the future might be tempted to think it was just an oversight and
add the call back in that you don't want.

======

14.
+# Despite inactive timeout being set, the synced slot won't get invalidated on
+# its own on the standby. So, we must not see invalidation message in server
+# log.
+$standby1->safe_psql('postgres', "CHECKPOINT");
+is( $standby1->safe_psql(
+ 'postgres',
+ q{SELECT count(*) = 1 FROM pg_replication_slots
+   WHERE slot_name = 'sync_slot1'
+ AND invalidation_reason IS NULL;}
+ ),
+ "t",
+ 'check that synced slot sync_slot1 has not been invalidated on standby');
+

But, now, we are confirming this by another way -- not checking the
logs here, so the comment "So, we must not see invalidation message in
server log." is no longer appropriate here.

======
[1] https://www.postgresql.org/message-id/flat/CAA4eK1JQFdssaBBh-oQskpKM-UpG8jPyUdtmGWa_0qCDy%2BK7_A%40mail.gmail.com#ab98379f220288ed40d34f8c2a21cf96

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: vignesh C <[email-masked]>
Date: Thu, 14 Nov 2024 09:14:40 +0530

> Fixed issues reported in [1], [2].

Few comments:
1) Since we don't change the value of now in
ReplicationSlotSetInactiveSince, the function parameter can be passed
by value:
+/*
+ * Set slot's inactive_since property unless it was previously invalidated.
+ */
+static inline void
+ReplicationSlotSetInactiveSince(ReplicationSlot *s, TimestampTz *now,
+                                                               bool
acquire_lock)
+{
+       if (s->data.invalidated != RS_INVAL_NONE)
+               return;
+
+       if (acquire_lock)
+               SpinLockAcquire(&s->mutex);
+
+       s->inactive_since = *now;

2) Currently it allows a minimum value of less than 1 second like in
milliseconds, I feel we can have some minimum value at least something
like checkpoint_timeout:
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 8a67f01200..367f510118 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -3028,6 +3028,18 @@ struct config_int ConfigureNamesInt[] =
                NULL, NULL, NULL
        },

+       {
+               {"replication_slot_inactive_timeout", PGC_SIGHUP,
REPLICATION_SENDING,
+                       gettext_noop("Sets the amount of time a
replication slot can remain inactive before "
+                                                "it will be invalidated."),
+                       NULL,
+                       GUC_UNIT_S
+               },
+               &replication_slot_inactive_timeout,
+               0, 0, INT_MAX,
+               NULL, NULL, NULL
+       },

3) Since SlotInactiveTimeoutCheckAllowed check is just done above and
the current time has been retrieved can we used "now" variable instead
of SlotInactiveTimeoutCheckAllowed again second time:
@@ -1651,6 +1713,26 @@
InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
                                        if (SlotIsLogical(s))
                                                invalidation_cause = cause;
                                        break;
+                               case RS_INVAL_INACTIVE_TIMEOUT:
+
+                                       /*
+                                        * Check if the slot needs to
be invalidated due to
+                                        *
replication_slot_inactive_timeout GUC.
+                                        */
+                                       if
(SlotInactiveTimeoutCheckAllowed(s) &&
+
TimestampDifferenceExceeds(s->inactive_since, now,
+
                            replication_slot_inactive_timeout * 1000))
+                                       {
+                                               invalidation_cause = cause;
+                                               inactive_since =
s->inactive_since;

4) I'm not sure if this change required by this patch or is it a
general optimization, if it is required for this patch we can detail
the comments:
@@ -2208,6 +2328,7 @@ RestoreSlotFromDisk(const char *name)
        bool            restored = false;
        int                     readBytes;
        pg_crc32c       checksum;
+       TimestampTz now;

        /* no need to lock here, no concurrent access allowed yet */

@@ -2368,6 +2489,9 @@ RestoreSlotFromDisk(const char *name)
                                                NameStr(cp.slotdata.name)),
                                 errhint("Change \"wal_level\" to be
\"replica\" or higher.")));

+       /* Use same inactive_since time for all slots */
+       now = GetCurrentTimestamp();
+
        /* nothing can be active yet, don't lock anything */
        for (i = 0; i < max_replication_slots; i++)
        {
@@ -2400,7 +2524,7 @@ RestoreSlotFromDisk(const char *name)
                 * slot from the disk into memory. Whoever acquires
the slot i.e.
                 * makes the slot active will reset it.
                 */
-               slot->inactive_since = GetCurrentTimestamp();
+               slot->inactive_since = now;

5) Why should the slot invalidation be updated during shutdown,
shouldn't the inactive_since value be intact during shutdown?
-        <literal>NULL</literal> if the slot is currently being used.
-        Note that for slots on the standby that are being synced from a
+        <literal>NULL</literal> if the slot is currently being used. Once the
+        slot is invalidated, this value will remain unchanged until we shutdown
+        the server. Note that for slots on the standby that are being
synced from a

6) New Style of ereport does not need braces around errcode, it can be
changed similarly:
+       if (error_if_invalid &&
+               s->data.invalidated == RS_INVAL_INACTIVE_TIMEOUT)
+       {
+               Assert(s->inactive_since > 0);
+               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 was inactive for longer than the amount of time
specified by \"%s\".",
+
"replication_slot_inactive_timeout")));

Regards,
Vignesh

---

From: Nisha Moond <[email-masked]>
Date: Tue, 19 Nov 2024 12:42:55 +0530

Attached is the v49 patch set:
- Fixed the bug reported in [1].
- Addressed comments in [2] and [3].

I've split the patch into two, implementing the suggested idea in
comment #5 of [2] separately in 001:

Patch-001: Adds additional error reports (for all invalidation types)
in ReplicationSlotAcquire() for invalid slots when error_if_invalid =
true.
Patch-002: The original patch with comments addressed.

~~~~

[1] https://www.postgresql.org/message-id/CALDaNm2mwkVFLfe8pLcU1W5Oy1vRr1Wzp53XGV08kr4Z2%3DSJpA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPt6s-qNPdxH5%3D-fr2QKLEv0h16sQ8EvLiGJ-SdQNS6pbw%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CALDaNm2VQW_gpOJ-QWkEA_h18DN31ELEz2_7QmwWCAg9%3DZew4A%40mail.gmail.com

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Tue, 19 Nov 2024 12:47:37 +0530

> the latest patch v48-0001.

Thank you for the review. Comments are addressed in v49 version.
Below is my response to comments that may require further discussion.

> you just clarifying the existing behaviour of the 'active_since'

Yes, this patch introduces inactive_timeout invalidation and prevents
updates to inactive_since for invalid slots. Only a node restart can
modify it,  so, I believe we should retain these lines in this patch.

> included there quickly before that other patch gets pushed.

Thanks for the reference, I have posted my suggestion on the thread.

> then there needs to be more ereports.

+1 to the idea.
I have created a separate patch v49-0001 adding more ereports for all
kinds of invalidations.

> Can this replication slot timeout due to inactivity?

I feel the suggestion is not very clear on the purpose of the
function, This function doesn't check inactivity or decide slot
timeout invalidation. It only pre-checks if the slot qualifies for an
inactivity check, which the caller will perform.
As I have changed function name too as per commnet#9, I used the following  -
"Is inactive timeout invalidation possible for this replication slot?"
Thoughts?

> tweaking because IMO that should also now be saying something about

'RecoveryInProgress' check indicates that the server is a standby, and
the mentioned paragraph uses the term "standby" to describe the
condition. It seems unnecessary to mention RecoveryInProgress
separately.

> introduction of this one in v48 looks out of place to me.

pgindent automatically added this blank line after 'case
RS_INVAL_INACTIVE_TIMEOUT'.

> IMO it would

Since it's not directly related to the patch, I feel it might be best
to leave it as is for now.

> 'SlotInactiveTimeoutCheckAllowed'?

I don't think it's a good idea to check (s->active_pid == 0) upfront,
before the timeout-invalidation check. AFAIU, this assertion is meant
to ensure active_pid = 0 only if the slot is going to be invalidated,
i.e., when the following condition is true:

replication_slot_inactive_timeout_sec * 1000)

Thoughts? Open to others' opinions too.

> to inactivity that active_id must be 0. e.g. Assert(s->active_pid ==

I don't fully understand the purpose of this change yet. I'll look
into it further and get back.

> add the call back in that you don't want.

Added comment saying avoid using ReplicationSlotSetInactiveSince()
here as it will skip the invalid slots.

~~~~

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Tue, 19 Nov 2024 12:50:51 +0530

> > Fixed issues reported in [1], [2].

Thanks for the review.

> +               &replication_slot_inactive_timeout,

Currently, the feature is disabled by default when
replication_slot_inactive_timeout = 0. However, if we set a minimum
value, the default_val cannot be less than min_val, making it
impossible to use 0 to disable the feature.
Thoughts or any suggestions?

> +               slot->inactive_since = now;

After removing the "ReplicationSlotSetInactiveSince" from here, it
became irrelevant to this patch. Now, it is a general optimization to
set the same timestamp for all slots while restoring from disk. I have
added a few comments as per Peter's suggestion.

> +        the server. Note that for slots on the standby that are being

The "inactive_since" data of a slot is not stored on disk, so the
older value cannot be restored after a restart.

--
Thanks,
Nisha

---

From: vignesh C <[email-masked]>
Date: Tue, 19 Nov 2024 15:53:35 +0530

> Patch-002: The original patch with comments addressed.

Few comments:
1) I felt this check in wait_for_slot_invalidation is not required as
there is a call to trigger_slot_invalidation which sleeps for
inactive_timeout seconds and ensures checkpoint is triggered, also the
test passes without this:
+       # Wait for slot to become inactive
+       $node->poll_query_until(
+               'postgres', qq[
+               SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+                       WHERE slot_name = '$slot' AND active = 'f' AND
+                                 inactive_since IS NOT NULL;
+       ])
+         or die
+         "Timed out while waiting for slot $slot to become inactive
on node $node_name";

2) Instead of calling this in a loop, won't it be enough to call
checkpoint only once explicitly:
+       for (my $i = 0; $i < 10 *
$PostgreSQL::Test::Utils::timeout_default; $i++)
+       {
+               $node->safe_psql('postgres', "CHECKPOINT");
+               if ($node->log_contains(
+                               "invalidating obsolete replication
slot \"$slot\"", $offset))
+               {
+                       $invalidated = 1;
+                       last;
+               }
+               usleep(100_000);
+       }
+       ok($invalidated,
+               "check that slot $slot invalidation has been logged on
node $node_name"
+       );

3) Since pg_sync_replication_slots is a sync call, we can directly use
"is( $standby1->safe_psql('postgres', SELECT COUNT(slot_name) = 1 FROM
pg_replication_slots..." instead of poll_query_until:
+$standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
+$standby1->poll_query_until(
+       'postgres', qq[
+       SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+               WHERE slot_name = 'sync_slot1' AND
+               invalidation_reason = 'inactive_timeout';
+])
+  or die
+  "Timed out while waiting for sync_slot1 invalidation to be synced
on standby";

4) Since this variable is being referred to at many places, how about
changing it to inactive_timeout_1s so that it is easier while
reviewing across many places:
# Set timeout GUC on the standby to verify that the next checkpoint will not
# invalidate synced slots.
my $inactive_timeout = 1;

5) Since we have already tested invalidation of logical replication
slot 'sync_slot1' above, this test might not be required:
+# =============================================================================
+# Testcase start
+# Invalidate logical subscriber slot due to inactive timeout.
+
+my $publisher = $primary;
+
+# Prepare for test
+$publisher->safe_psql(
+       'postgres', qq[
+    ALTER SYSTEM SET replication_slot_inactive_timeout TO '0';
+]);
+$publisher->reload;

Regards,
Vignesh

---

From: vignesh C <[email-masked]>
Date: Wed, 20 Nov 2024 13:29:29 +0530

> Patch-002: The original patch with comments addressed.

This Assert can fail:
+                                       /*
+                                        * Check if the slot needs to
be invalidated due to
+                                        *
replication_slot_inactive_timeout GUC.
+                                        */
+                                       if (now &&
+
TimestampDifferenceExceeds(s->inactive_since, now,
+
                            replication_slot_inactive_timeout_sec *
1000))
+                                       {
+                                               invalidation_cause = cause;
+                                               inactive_since =
s->inactive_since;
+
+                                               /*
+                                                * Invalidation due to
inactive timeout implies that
+                                                * no one is using the slot.
+                                                */
+                                               Assert(s->active_pid == 0);

With the following scenario:
Set replication_slot_inactive_timeout to 10 seconds
-- Create a slot
postgres=# select pg_create_logical_replication_slot ('test',
'pgoutput', true, true);
 pg_create_logical_replication_slot
------------------------------------
 (test,0/1748068)
(1 row)

-- Wait for 10 seconds and execute checkpoint
postgres=# checkpoint;
WARNING:  terminating connection because of crash of another server process
DETAIL:  The postmaster has commanded this server process to roll back
the current transaction and exit, because another server process
exited abnormally and possibly corrupted shared memory.
HINT:  In a moment you should be able to reconnect to the database and
repeat your command.
server closed the connection unexpectedly

The assert fails:
#5  0x00005b074f0c922f in ExceptionalCondition
(conditionName=0x5b074f2f0b4c "s->active_pid == 0",
fileName=0x5b074f2f0010 "slot.c", lineNumber=1762) at assert.c:66
#6  0x00005b074ee26ead in InvalidatePossiblyObsoleteSlot
(cause=RS_INVAL_INACTIVE_TIMEOUT, s=0x740925361780, oldestLSN=0,
dboid=0, snapshotConflictHorizon=0, invalidated=0x7fffaee87e63) at
slot.c:1762
#7  0x00005b074ee273b2 in InvalidateObsoleteReplicationSlots
(cause=RS_INVAL_INACTIVE_TIMEOUT, oldestSegno=0, dboid=0,
snapshotConflictHorizon=0) at slot.c:1952
#8  0x00005b074ee27678 in CheckPointReplicationSlots
(is_shutdown=false) at slot.c:2061
#9  0x00005b074e9dfda7 in CheckPointGuts (checkPointRedo=24412528,
flags=108) at xlog.c:7513
#10 0x00005b074e9df4ad in CreateCheckPoint (flags=108) at xlog.c:7179
#11 0x00005b074edc6bfc in CheckpointerMain (startup_data=0x0,
startup_data_len=0) at checkpointer.c:463

Regards,
Vignesh

---

From: vignesh C <[email-masked]>
Date: Thu, 21 Nov 2024 10:36:32 +0530

> Thoughts or any suggestions?

We could implement this similarly to how the vacuum_buffer_usage_limit
GUC is handled. Setting the value to 0 would allow the operation to
use any amount of shared_buffers. Otherwise, valid sizes would range
from 128 kB to 16 GB. Similarly, we can modify
check_replication_slot_inactive_timeout to behave in the same way as
check_vacuum_buffer_usage_limit function.

Regards,
Vignesh

---

From: Nisha Moond <[email-masked]>
Date: Thu, 21 Nov 2024 17:35:01 +0530

> > Patch-002: The original patch with comments addressed.

Attached v50 patch-set addressing review comments in [1] and [2].

Regarding the assert issue reported in [2]:
- For temporary replication slots, the current session's pid serves as
the active_pid for the slot, which is expected behavior.
- Therefore, the ASSERT has been removed in v50. Now, if a temporary
slot qualifies for a timeout invalidation, the holding process will be
terminated, and the slot will be invalidated.

[1] https://www.postgresql.org/message-id/CALDaNm2UUTfJczjR-rEQwKgmx%3DiFnuMnR1cXv7ccB%2BO9P15mYg%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALDaNm0g86wD2%3DbQdFOy0smsP0MZWyz0CUqXej%3DQi-hCEeqkag%40mail.gmail.com

--
Thanks,
Nisha

---

From: vignesh C <[email-masked]>
Date: Fri, 22 Nov 2024 17:43:24 +0530

> Attached v50 patch-set addressing review comments in [1] and [2].

We are setting inactive_since when the replication slot is released.
We are marking the slot as inactive only if it has been released.
However, there's a scenario where the network connection between the
publisher and subscriber may be lost where the replication slot is not
released, but no changes are replicated due to the network problem. In
this case, no updates would occur in the replication slot for a period
exceeding the replication_slot_inactive_timeout.
Should we invalidate these replication slots as well, or is it
intentionally left out?

Regards,
Vignesh

---

From: Peter Smith <[email-masked]>
Date: Wed, 27 Nov 2024 14:09:15 +1100

Hi Nisha,

Here are some review comments for the patch v50-0002.

======

1.
+ if (now &&
+ TimestampDifferenceExceeds(s->inactive_since, now,
+    replication_slot_inactive_timeout_sec * 1000))

Previously this was using an additional call to SlotInactiveTimeoutCheckAllowed:

Is it OK to skip that call? e.g. can the slot fields possibly change
between assigning the 'now' and acquiring the mutex? If not, then the
current code is fine. The only reason for asking is because it is
slightly suspicious that it was not done this "easy" way in the first
place.

~~~

check_replication_slot_inactive_timeout:

2.
+/*
+ * GUC check_hook for replication_slot_inactive_timeout
+ *
+ * We don't allow the value of replication_slot_inactive_timeout other than 0
+ * during the binary upgrade.
+ */

The "We don't allow..." sentence seems like a backward way of saying:
The value of replication_slot_inactive_timeout must be set to 0 during
the binary upgrade.

======

3.
+# Despite inactive timeout being set, the synced slot won't get invalidated on
+# its own on the standby.

What does "on its own" mean here? Do you mean it won't get invalidated
unless the invalidation state is propagated from the primary? Maybe
the comment can be clearer.

~

4.
+# Wait for slot to first become inactive and then get invalidated
+sub wait_for_slot_invalidation
+{
+ my ($node, $slot, $offset, $inactive_timeout_1s) = @_;
+ my $node_name = $node->name;
+

It was OK to change the variable name to 'inactive_timeout_1s' outside
of here, but within the subroutine, I don't think it is appropriate
because this is a parameter that potentially could have any value.

~

5.
+# Trigger slot invalidation and confirm it in the server log
+sub trigger_slot_invalidation
+{
+ my ($node, $slot, $offset, $inactive_timeout_1s) = @_;
+ my $node_name = $node->name;
+ my $invalidated = 0;

It was OK to change the variable name to 'inactive_timeout_1s' outside
of here, but within the subroutine, I don't think it is appropriate
because this is a parameter that potentially could have any value.

~

6.
+ # Give enough time to avoid multiple checkpoints
+ sleep($inactive_timeout_1s + 1);
+
+ # Run a checkpoint
+ $node->safe_psql('postgres', "CHECKPOINT");

Since you are not doing multiple checkpoints anymore, it looks like
that "Give enough time..." comment needs updating.

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

---

From: Nisha Moond <[email-masked]>
Date: Wed, 27 Nov 2024 16:24:49 +0530

> slightly suspicious that it was not done this "easy" way in the first
Good catch! While the mutex was being acquired right after the now
assignment, there was a rare chance of another process modifying the
slot in the meantime. So, I reverted the change in v51. To optimize
the SlotInactiveTimeoutCheckAllowed() call, it's sufficient to check
it here instead of during the 'now' assignment.

Attached v51 patch-set addressing all comments in [1] and [2].

[1] https://www.postgresql.org/message-id/CAHut%2BPtuiQj1hwm%3D73xJ8hWuw-9cXbN4dHJHpM6EXxubDJgmFA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPvi-g%2B9%2Bhjmjg44OzTN9L3YGQiCXBDAVaTVWvSn5SSwmw%40mail.gmail.com

--
Thanks,
Nisha

---

From: Peter Smith <[email-masked]>
Date: Thu, 28 Nov 2024 08:37:47 +1100

Hi Nisha, here are my review comments for the patch v51-0001.

======

ReplicationSlotAcquire:

1.
+ 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));
+
+ pfree(err_detail.data);
+ }
+

Won't the 'pfree' be unreachable due to the prior ereport ERROR?

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

---

From: Peter Smith <[email-masked]>
Date: Thu, 28 Nov 2024 10:50:05 +1100

Hi Nisha. Here are some review comments for patch v51-0002.

======

1.
         The time when the slot became inactive. <literal>NULL</literal> if the
-        slot is currently being streamed.
+        slot is currently being streamed. Once the slot is invalidated, this
+        value will remain unchanged until we shutdown the server.
.

I think "Once the ..." kind of makes it sound like invalidation is
inevitable. Also maybe it's better to remove the "we".

SUGGESTION:
If the slot becomes invalidated, this value will remain unchanged
until server shutdown.

======

ReplicationSlotAcquire:

2.
GENERAL.

This just is a question/idea. It may not be feasible to change. It
seems like there is a lot of overlap between the error messages in
'ReplicationSlotAcquire' which are saying "This slot has been
invalidated because...", and with the other function
'ReportSlotInvalidation' which is kind of the same but called in
different circumstances and with slightly different message text. I
wondered if there is a way to use common code to unify these messages
instead of having a nearly duplicate set of messages for all the
invalidation causes?

~~~

3.
+ case RS_INVAL_INACTIVE_TIMEOUT:
+ appendStringInfo(&err_detail, _("inactivity exceeded the time limit
set by \"%s\"."),
+ "replication_slot_inactive_timeout");
+ break;

Should this err_detail also say "This slot has been invalidated
because ..." like all the others?

~~~

4.
+ if (IsSlotInactiveTimeoutPossible(s) &&

Maybe this code should have Assert(now > 0); before the condition just
as a way to 'document' that it is assumed 'now' was already set this
outside the mutex.

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 28 Nov 2024 07:59:21 +0000

Dear Nisha,

> Attached v51 patch-set addressing all comments in [1] and [2].

Thanks for working on the feature! I've stated to review the patch.
Here are my comments - sorry if there are something which have already been discussed.
The thread is too long to follow correctly.

Comments for 0001
=============

01. binary_upgrade_logical_slot_has_caught_up

ISTM that error_if_invalid is set to true when the slot can be moved forward, otherwise
it is set to false. Regarding the binary_upgrade_logical_slot_has_caught_up, however,
only valid slots will be passed to the funciton (see pg_upgrade/info.c) so I feel
it is OK to set to true. Thought?

02. ReplicationSlotAcquire

According to other functions, we are adding to a note to the translator when
parameters represent some common nouns, GUC names. I feel we should add a comment
for RS_INVAL_WAL_LEVEL part based on it.

Comments for 0002
=============

03. check_replication_slot_inactive_timeout

Can we overwrite replication_slot_inactive_timeout to zero when pg_uprade (and also
pg_createsubscriber?) starts a server process? Several parameters have already been
specified via -c option at that time. This can avoid an error while the upgrading.
Note that this part is still needed even if you accept the comment. Users can
manually boot with upgrade mode.

04. ReplicationSlotAcquire

Same comment as 02.

05. ReportSlotInvalidation

Same comment as 02.

06. found bug

While testing the patch, I found that slots can be invalidated too early when when
the GUC is quite large. I think because an overflow is caused in InvalidatePossiblyObsoleteSlot().

- Reproducer

I set the replication_slot_inactive_timeout to INT_MAX and executed below commands,
and found that the slot is invalidated.

```
postgres=# SHOW replication_slot_inactive_timeout;
 replication_slot_inactive_timeout 
-----------------------------------
 2147483647s
(1 row)
postgres=# SELECT * FROM pg_create_logical_replication_slot('test', 'test_decoding');
 slot_name |    lsn    
-----------+-----------
 test      | 0/18B7F38
(1 row)
postgres=# CHECKPOINT ;
CHECKPOINT
postgres=# SELECT slot_name, inactive_since, invalidation_reason FROM pg_replication_slots ;
 slot_name |        inactive_since         | invalidation_reason 
-----------+-------------------------------+---------------------
 test      | 2024-11-28 07:50:25.927594+00 | inactive_timeout
(1 row)
```

- analysis

In InvalidatePossiblyObsoleteSlot(), replication_slot_inactive_timeout_sec * 1000
is passed to the third argument of TimestampDifferenceExceeds(), which is also the
integer datatype. This causes an overflow and parameter is handled as the small
value.

- solution

I think there are two possible solutions. You can choose one of them:

a. Make the maximum INT_MAX/1000, or
b. Change the unit to millisecond.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: vignesh C <[email-masked]>
Date: Thu, 28 Nov 2024 14:43:50 +0530

> Should we invalidate these replication slots as well, or is it

On further thinking, I felt we can keep the current implementation as
is and simply add a brief comment in the code to address this.
Additionally, we can mention it in the commit message for clarity.

Regards,
Vignesh

---

From: vignesh C <[email-masked]>
Date: Thu, 28 Nov 2024 19:40:32 +0530

> Attached v51 patch-set addressing all comments in [1] and [2].

Few comments:
1) replication_slot_inactive_timeout can be mentioned in logical
replication config, we could mention something like:
Logical replication slot is also affected by replication_slot_inactive_timeout

2.a) Is this change applicable only for inactive timeout or it is
applicable to others like wal removed, wal level etc also? If it is
applicable to all of them we could move this to the first patch and
update the commit message:
+                * If the slot can be acquired, do so and mark it as
invalidated. If
+                * the slot is already ours, mark it as invalidated.
Otherwise, we'll
+                * signal the owning process below and retry.
                 */
-               if (active_pid == 0)
+               if (active_pid == 0 ||
+                       (MyReplicationSlot == s &&
+                        active_pid == MyProcPid))

2.b) Also this MyReplicationSlot and active_pid check can be in same line:
+                       (MyReplicationSlot == s &&
+                        active_pid == MyProcPid))

3) Error detail should start in upper case here similar to how others are done:
+                       case RS_INVAL_INACTIVE_TIMEOUT:
+                               appendStringInfo(&err_detail,
_("inactivity exceeded the time limit set by \"%s\"."),
+
"replication_slot_inactive_timeout");
+                               break;

4) Since this change is not related to this patch, we can move this to
the first patch and update the commit message:
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -1508,7 +1508,7 @@ ReplSlotSyncWorkerMain(char *startup_data,
size_t startup_data_len)
 static void
 update_synced_slots_inactive_since(void)
 {
-       TimestampTz now = 0;
+       TimestampTz now;

        /*
         * We need to update inactive_since only when we are promoting
standby to
@@ -1523,6 +1523,9 @@ update_synced_slots_inactive_since(void)
        /* The slot sync worker or SQL function mustn't be running by now */
        Assert((SlotSyncCtx->pid == InvalidPid) && !SlotSyncCtx->syncing);

5) Since this change is not related to this patch, we can move this to
the first patch.
@@ -2250,6 +2350,7 @@ RestoreSlotFromDisk(const char *name)
        bool            restored = false;
        int                     readBytes;
        pg_crc32c       checksum;
+       TimestampTz now;

@@ -2410,6 +2511,9 @@ RestoreSlotFromDisk(const char *name)
                                                NameStr(cp.slotdata.name)),
                                 errhint("Change \"wal_level\" to be
\"replica\" or higher.")));

+       /* Use same inactive_since time for all slots */
+       now = GetCurrentTimestamp();
+
        /* nothing can be active yet, don't lock anything */
        for (i = 0; i < max_replication_slots; i++)
        {
@@ -2440,9 +2544,11 @@ 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();
+               slot->inactive_since = now;

[1] - https://www.postgresql.org/docs/current/logical-replication-config.html

Regards,
Vignesh

---

From: Nisha Moond <[email-masked]>
Date: Fri, 29 Nov 2024 18:06:02 +0530

> Additionally, we can mention it in the commit message for clarity.

Thank you for the clarification. I’ve included the explanatory comment
in patch-002.

Attached the v52 patch-set addressing above as well as all other
comments till now in [1], [2], [3], and [4].

[1] https://www.postgresql.org/message-id/CAHut%2BPto1Yz9Fqp07LLP9uvx3sRHe5SOUKuFM1sUF9QA5aLfBA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPs%3DH6EBO1ssGfykrJfUQQGh76L0eKuU5XkR9GMs96ZT3g%40mail.gmail.com
[3] https://www.postgresql.org/message-id/TYAPR01MB56927564EEE26E5433198405F5292%40TYAPR01MB5692.jpnprd01.prod.outlook.com
[4] https://www.postgresql.org/message-id/CALDaNm1F2YrswzM_WM37BYmiZ9Cf60UD_mgtm8HnMHRGA7tx4g%40mail.gmail.com

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Fri, 29 Nov 2024 18:06:33 +0530

> into it further and get back.

This change applies to all types of invalidation, not just
inactive_timeout case, so moved the change to patch-001. It’s a
general optimization for the case when the current process is the
active PID for the slot.
Also, the Assert(s->active_pid == 0); has been removed (in v50) as it
was unnecessary.

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Fri, 29 Nov 2024 18:06:56 +0530

> The thread is too long to follow correctly.

Right, corrected the call with error_if_invalid as true.

Done.

>  replication_slot_inactive_timeout

Fixed. It is reasonable to align with other timeout parameters by
using milliseconds as the unit.

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Fri, 29 Nov 2024 18:07:32 +0530

> instead of having a nearly duplicate set of messages for all the

The error handling could be moved to a new function; however, as you
pointed out, the contexts in which these functions are called differ.
IMO, a single error message may not suit both cases. For example,
ReportSlotInvalidation provides additional details and a hint in its
message, which isn’t necessary for ReplicationSlotAcquire.
Thoughts?

--
Thanks,
Nisha

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 3 Dec 2024 07:39:13 +0000

Dear Nisha,

Thanks for updating the patch!

> using milliseconds as the unit.

It looks you just replaced to GUC_UNIT_MS, but the documentation and
postgresql.conf.sample has not been changed yet. They should follow codes.
Anyway, here are other comments, mostly cosmetic.

01. slot.c

```
+int         replication_slot_inactive_timeout_ms = 0;
```

According to other lines, we should add a short comment for the GUC.

02. 050_invalidate_slots.pl

Do you have a reason why you use the number 050? I feel it can be 043.

03. 050_invalidate_slots.pl

Also, not sure the file name is correct. This file contains only a slot invalidation due to the
replication_slot_inactive_timeout. But I feel current name is too general.

04. 050_invalidate_slots.pl

```
+use Time::HiRes qw(usleep);
```

This line is not needed because usleep() is not used in this file.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Nisha Moond <[email-masked]>
Date: Wed, 4 Dec 2024 15:00:52 +0530

> Anyway, here are other comments, mostly cosmetic.

Here is v53 patch-set addressing all the comments in [1] and [2].

[1] https://www.postgresql.org/message-id/CAHut%2BPsQM79f34LLBGq4UeRuZ1URWP6JNZtdN2khYPrLc1YqrQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/TYAPR01MB5692B7687EE7981AA91BA5B9F5362%40TYAPR01MB5692.jpnprd01.prod.outlook.com

--
Thanks,
Nisha

---

From: vignesh C <[email-masked]>
Date: Wed, 4 Dec 2024 15:56:53 +0530

> Here is v53 patch-set addressing all the comments in [1] and [2].

Currently, replication slots are invalidated based on the
replication_slot_inactive_timeout only during a checkpoint. This means
that if the checkpoint_timeout is set to a higher value than the
replication_slot_inactive_timeout, slot invalidation will occur only
when the checkpoint is triggered. Identifying the invalidation slots
might be slightly delayed in this case. As an alternative, users can
forcefully invalidate inactive slots that have exceeded the
replication_slot_inactive_timeout by forcing a checkpoint. I was
thinking we could suggest this in the documentation.

+       <para>
+        Slot invalidation due to inactive timeout occurs during checkpoint.
+        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>
+        value.
+       </para>
+

We could accurately invalidate the slots using the checkpointer
process by calculating the invalidation time based on the active_since
timestamp and the replication_slot_inactive_timeout, and then set the
checkpointer's main wait-latch accordingly for triggering the next
checkpoint. Ideally, a different process handling this task would be
better, but there is currently no dedicated daemon capable of
identifying and managing slots across streaming replication, logical
replication, and other slots used by plugins. Additionally,
overloading the checkpointer with this responsibility may not be
ideal. As an alternative, we could document about this delay in
identifying and mention that it could be triggered by forceful manual
checkpoint.

Regards,
Vignesh

---

From: Peter Smith <[email-masked]>
Date: Thu, 5 Dec 2024 12:13:40 +1100

On Wed, Dec 4, 2024 at 9:27 PM vignesh C <[email-masked]> wrote:
...
> identifying and mention that it could be triggered by forceful manual

Hi Vignesh.

I felt that manipulating the checkpoint timing behind the scenes
without the user's consent might be a bit of an overreach.

But there might still be something else we could do:

1. We can add the documentation note like you suggested ("we could
document about this delay in identifying and mention that it could be
triggered by forceful manual checkpoint").

2. We can also detect such delays in the code. When the invalidation
occurs (e.g. code fragment below) we could check if there was some
excessive lag between the slot becoming idle and it being invalidated.
If the lag is too much (whatever "too much" means) we can log a hint
for the user to increase the checkpoint frequency (or whatever else we
might advise them to do).

+ /*
+ * Check if the slot needs to be invalidated due to
+ * replication_slot_inactive_timeout GUC.
+ */
+ if (IsSlotInactiveTimeoutPossible(s) &&
+ TimestampDifferenceExceeds(s->inactive_since, now,
+    replication_slot_inactive_timeout_ms))
+ {
+ invalidation_cause = cause;
+ inactive_since = s->inactive_since;

pseudo-code:
if (slot invalidation occurred much later after the
replication_slot_inactive_timeout GUC elapsed)
{
  elog(LOG, "This slot was inactive for a period of %s. Slot timeout
invalidation only occurs at a checkpoint so if you want inactive slots
to be invalidated in a more timely manner consider reducing the time
between checkpoints or executing a manual checkpoint.
(replication_slot_inactive_timeout = %s; checkpoint_timeout = %s,
....)"
}

+ }

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

---

From: vignesh C <[email-masked]>
Date: Fri, 6 Dec 2024 11:04:29 +0530

> without the user's consent might be a bit of an overreach.

Agree

> triggered by forceful manual checkpoint").

Yes, that makes sense

> (replication_slot_inactive_timeout = %s; checkpoint_timeout = %s,

Determining the correct time may be challenging for users, as it
depends on when the active_since value is set, as well as when the
checkpoint_timeout occurs and the subsequent checkpoint is triggered.
Even if the user sets it to an appropriate value, there is still a
possibility of delayed identification due to the timing of when the
slot's active_timeout is being set. Including this information in the
documentation should be sufficient.

Regards,
Vignesh

---

From: Nisha Moond <[email-masked]>
Date: Tue, 10 Dec 2024 17:21:09 +0530

> documentation should be sufficient.

+1
v54 documents this information as suggested.

Attached the v54 patch-set addressing all the comments till now in
[1], [2] and [3].

[1] https://www.postgresql.org/message-id/CALDaNm0mTWwg0z4v-sorq08S2CdZmL2s%2Brh4nHpWeJaBQ2F%2Bmg%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALDaNm1STyk%3DS_EAihWP9SowBkS5dJ32JfEqmG5tTeC2Ct39yg%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CAHut%2BPtHbYNxPvtMfs7jARbsVcFXL1%3DC9SO3Q93NgVDgbKN7LQ%40mail.gmail.com

--
Thanks,
Nisha

---

From: Peter Smith <[email-masked]>
Date: Wed, 11 Dec 2024 13:44:05 +1100

Hi Nisha.

Here are some review comments for patch v54-0002.

(I had also checked patch v54-0001, but have no further review
comments for that one).

======
doc/src/sgml/config.sgml

1.
+       <para>
+        Slot invalidation due to idle timeout occurs during checkpoint.
+        If the <varname>checkpoint_timeout</varname> exceeds
+        <varname>idle_replication_slot_timeout</varname>, the slot
+        invalidation will be delayed until the next checkpoint is triggered.
+        To avoid delays, users can force a checkpoint to promptly invalidate
+        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>
+        value.
+       </para>
+

The wording of "If the checkpoint_timeout exceeds
idle_replication_slot_timeout, the slot invalidation will be delayed
until the next checkpoint is triggered." seems slightly misleading,
because AFAIK it is not conditional on the GUC value differences like
that -- i.e. slot invalidation is *always* delayed until the next
checkpoint occurs.

SUGGESTION:
Slot invalidation due to idle timeout occurs during checkpoint.
Because checkpoints happen at checkpoint_timeout intervals, there can
be some lag between when the idle_replication_slot_timeout was
exceeded and when the slot invalidation is triggered at the next
checkpoint. To avoid such lags, users can force...

=======

2. GENERAL

+/* Invalidate replication slots idle beyond this time; '0' disables it */
+int idle_replication_slot_timeout_ms = 0;

I noticed this patch is using a variety of ways of describing the same thing:
* guc var: Invalidate replication slots idle beyond this time...
* guc_tables: ... the amount of time a replication slot can remain
idle before it will be invalidated.
* docs: means that the slot has remained idle beyond the duration
specified by the idle_replication_slot_timeout parameter
* errmsg: ... slot has been invalidated because inactivity exceeded
the time limit set by ...
* etc..

They are all the same, but they are all worded slightly differently:
* "idle" vs "inactivity" vs ...
* "time" vs "amount of time" vs "duration" vs "time limit" vs ...

There may not be a one-size-fits-all, but still, it might be better to
try to search for all different phrasing and use common wording as
much as possible.

~~~

CheckPointReplicationSlots:

3.
+ * XXX: Slot invalidation due to 'idle_timeout' occurs only for
+ * released slots, based on 'idle_replication_slot_timeout'. Active
+ * slots in use for replication are excluded, preventing accidental
+ * invalidation. Slots where communication between the publisher and
+ * subscriber is down are also excluded, as they are managed by the
+ * 'wal_sender_timeout'.

Maybe a slight rewording like below is better. Maybe not. YMMV.

SUGGESTION:
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...

======
src/bin/pg_upgrade/server.c

4.
+ /*
+ * Use idle_replication_slot_timeout=0 to prevent slot invalidation due to
+ * inactive_timeout by checkpointer process during upgrade.
+ */
+ if (GET_MAJOR_VERSION(cluster->major_version) >= 1800)
+ appendPQExpBufferStr(&pgoptions, " -c idle_replication_slot_timeout=0");
+

/inactive_timeout/idle_timeout/

======
src/test/recovery/t/043_invalidate_inactive_slots.pl

5.
+# Wait for slot to first become idle and then get invalidated
+sub wait_for_slot_invalidation
+{
+ my ($node, $slot, $offset, $idle_timeout) = @_;
+ my $node_name = $node->name;

AFAICT this 'idle_timeout' parameter is passed units of "seconds", so
it would be better to call it something like 'idle_timeout_s' to make
the units clear.

~~~

6.
+# Trigger slot invalidation and confirm it in the server log
+sub trigger_slot_invalidation
+{
+ my ($node, $slot, $offset, $idle_timeout) = @_;
+ my $node_name = $node->name;
+ my $invalidated = 0;

Ditto above review comment #5 -- better to call it something like
'idle_timeout_s' to make the units clear.

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

---

From: vignesh C <[email-masked]>
Date: Thu, 12 Dec 2024 09:42:35 +0530

> Attached the v54 patch-set addressing all the comments till now in

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:
+# Set timeout GUC on the standby to verify that the next checkpoint will not
+# invalidate synced slots.
+my $idle_timeout_1s = 1;
+$standby1->safe_psql(
+       'postgres', qq[
+    ALTER SYSTEM SET idle_replication_slot_timeout TO '${idle_timeout_1s}s';
+]);
+$standby1->reload;
+
+# Sync the primary slots to the standby
+$standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
+
+# Confirm that the logical failover slot is created on the standby
+is( $standby1->safe_psql(
+               'postgres',
+               q{SELECT count(*) = 1 FROM pg_replication_slots
+                 WHERE slot_name = 'sync_slot1' AND synced
+                       AND NOT temporary
+                       AND invalidation_reason IS NULL;}
+       ),
+       "t",
+       'logical slot sync_slot1 is synced to standby');
+
+# Give enough time for inactive_since to exceed the timeout
+sleep($idle_timeout_1s + 1);

Regards,
Vignesh