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

From: Amit Kapila <[email-masked]>
Date: Tue, 26 Mar 2024 13:37:21 +0530

On Tue, Mar 26, 2024 at 1:15 PM Bertrand Drouvot
<[email-masked]> wrote:
>
> 2 ===
>
> It looks like inactive_since is set to the current timestamp on the standby
> each time the sync worker does a cycle:
>
> primary:
>
> postgres=# select slot_name,inactive_since from pg_replication_slots where failover = 't';
>   slot_name  |        inactive_since
> -------------+-------------------------------
>  lsub27_slot | 2024-03-26 07:39:19.745517+00
>  lsub28_slot | 2024-03-26 07:40:24.953826+00
>
> standby:
>
> postgres=# select slot_name,inactive_since from pg_replication_slots where failover = 't';
>   slot_name  |        inactive_since
> -------------+-------------------------------
>  lsub27_slot | 2024-03-26 07:43:56.387324+00
>  lsub28_slot | 2024-03-26 07:43:56.387338+00
>
> I don't think that should be the case.
>

But why? This is exactly what we discussed in another thread where we
agreed to update inactive_since even for sync slots. In each sync
cycle, we acquire/release the slot, so the inactive_since gets
updated. See synchronize_one_slot().

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 26 Mar 2024 08:24:00 +0000

Hi,

On Tue, Mar 26, 2024 at 01:37:21PM +0530, Amit Kapila wrote:
> agreed to update inactive_since even for sync slots.

Hum, I thought we agreed to "sync" it and to "update it to current time"
only at promotion time.

I don't think updating inactive_since to current time during each cycle makes
sense (I mean I understand the use case: being able to say when slots have been
sync, but if this is what we want then we should consider an extra view or an
extra field but not relying on the inactive_since one).

If the primary goes down, not updating inactive_since to the current time could
also provide benefit such as knowing the inactive_since of the primary slots
(from the standby) the last time it has been synced. If we update it to the current
time then this information is lost.

> In each sync

Right, and I think we should put an extra condition if in recovery.

Regards,

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

---

From: shveta malik <[email-masked]>
Date: Tue, 26 Mar 2024 15:17:36 +0530

> Hum, I thought we agreed to "sync" it and to "update it to current time"

I think there may have been some misunderstanding here. But now if I
rethink this, I am fine with 'inactive_since' getting synced from
primary to standby. But if we do that, we need to add docs stating
"inactive_since" represents primary's inactivity and not standby's
slots inactivity for synced slots. The reason for this clarification
is that the synced slot might be generated much later, yet
'inactive_since' is synced from the primary, potentially indicating a
time considerably earlier than when the synced slot was actually
created.

Another approach could be that "inactive_since" for synced slot
actually gives its own inactivity data rather than giving primary's
slot data. We update inactive_since on standby only at 3 occasions:
1) at the time of creation of the synced slot.
2) during standby restart.
3) during promotion of standby.

I have attached a sample patch for this idea as.txt file.

I am fine with any of these approaches.  One gives data synced from
primary for synced slots, while another gives actual inactivity data
of synced slots.

thanks
Shveta
From 7dcd0e95299263187eb1f03812f8321b2612ee5c Mon Sep 17 00:00:00 2001
From: Shveta Malik <[email-masked]>
Date: Tue, 26 Mar 2024 14:42:25 +0530
Subject: [PATCH v1] inactive_since for synced slots.

inactive_since is updated for synced slots:
1) at the time of creation of slot.
2) during server restart.
3) during promotion.
---
 src/backend/replication/logical/slotsync.c |  1 +
 src/backend/replication/slot.c             | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index bbf9a2c485..6114895dca 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -628,6 +628,7 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid)
 		SpinLockAcquire(&slot->mutex);
 		slot->effective_catalog_xmin = xmin_horizon;
 		slot->data.catalog_xmin = xmin_horizon;
+		slot->inactive_since = GetCurrentTimestamp();
 		SpinLockRelease(&slot->mutex);
 		ReplicationSlotsComputeRequiredXmin(true);
 		LWLockRelease(ProcArrayLock);
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index d0a2f440ef..f2a57a14ec 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -628,7 +628,10 @@ retry:
 	 * now.
 	 */
 	SpinLockAcquire(&s->mutex);
-	s->inactive_since = 0;
+
+	if (!(RecoveryInProgress() && s->data.synced))
+		s->inactive_since = 0;
+
 	SpinLockRelease(&s->mutex);
 
 	if (am_walsender)
@@ -704,14 +707,20 @@ ReplicationSlotRelease(void)
 		 */
 		SpinLockAcquire(&slot->mutex);
 		slot->active_pid = 0;
-		slot->inactive_since = now;
+
+		if (!(RecoveryInProgress() && slot->data.synced))
+			slot->inactive_since = now;
+
 		SpinLockRelease(&slot->mutex);
 		ConditionVariableBroadcast(&slot->active_cv);
 	}
 	else
 	{
 		SpinLockAcquire(&slot->mutex);
-		slot->inactive_since = now;
+
+		if (!(RecoveryInProgress() && slot->data.synced))
+			slot->inactive_since = now;
+
 		SpinLockRelease(&slot->mutex);
 	}
 
-- 
2.34.1

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 26 Mar 2024 10:20:50 +0000

Hi,

On Tue, Mar 26, 2024 at 03:17:36PM +0530, shveta malik wrote:
> I think there may have been some misunderstanding here.

Indeed ;-)

> But now if I
> slots inactivity for synced slots.

Yeah sure.

> The reason for this clarification

Right.

> I have attached a sample patch for this idea as.txt file.

Thanks!

> primary for synced slots, while another gives actual inactivity data

What about another approach?: inactive_since gives data synced from primary for
synced slots and another dedicated field (could be added later...) could
represent what you suggest as the other option.

Another cons of updating inactive_since at the current time during each slot
sync cycle is that calling GetCurrentTimestamp() very frequently
(during each sync cycle of very active slots) could be too costly.

Regards,

---

From: shveta malik <[email-masked]>
Date: Tue, 26 Mar 2024 16:17:53 +0530

> represent what you suggest as the other option.

Yes, okay with me. I think there is some confusion here as well. In my
second approach above, I have not suggested anything related to
sync-worker. We can think on that later if we really need another
field which give us sync time.  In my second approach, I have tried to
avoid updating inactive_since for synced slots during sync process. We
update that field during creation of synced slot so that
inactive_since reflects correct info even for synced slots (rather
than copying from primary). Please have a look at my patch and let me
know your thoughts. I am fine with copying it from primary as well and
documenting this behaviour.

Right.

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 26 Mar 2024 16:35:16 +0530

> documenting this behaviour.

I took a look at your patch.

@@ -628,6 +628,7 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid
remote_dbid)

If we just sync inactive_since value for synced slots while in
recovery from the primary, so be it. Why do we need to update it to
the current time when the slot is being created? We don't expose slot
creation time, no? Aren't we fine if we just sync the value from
primary and document that fact? After the promotion, we can reset it
to the current time so that it gets its own time. Do you see any
issues with it?

-- 
Bharath Rupireddy

---

From: shveta malik <[email-masked]>
Date: Tue, 26 Mar 2024 16:49:18 +0530

On Tue, Mar 26, 2024 at 4:35 PM Bharath Rupireddy
<[email-masked]> wrote:
> the current time when the slot is being created?

If we update inactive_since  at synced slot's creation or during
restart (skipping setting it during sync), then this time reflects
actual 'inactive_since' for that particular synced slot.  Isn't that a
clear info for the user and in alignment of what the name
'inactive_since' actually suggests?

> We don't expose slot
> creation time, no?

No, we don't. But for synced slot, that is the time since that slot is
inactive  (unless promoted), so we are exposing inactive_since and not
creation time.

>Aren't we fine if we just sync the value from

Yes, we can do that. But curious to know, do we see any additional
benefit of reflecting primary's inactive_since at standby which I
might be missing?

thanks
Shveta

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 26 Mar 2024 12:31:08 +0000

Hi,

> benefit of reflecting primary's inactive_since at standby which I

In case the primary goes down, then one could use the value on the standby
to get the value coming from the primary. I think that could be useful info to
have.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 26 Mar 2024 12:35:04 +0000

Hi,

On Tue, Mar 26, 2024 at 04:17:53PM +0530, shveta malik wrote:
> sync-worker.

Yeah, no confusion, understood that way.

> We can think on that later if we really need another
> field which give us sync time.

I think that calling GetCurrentTimestamp() so frequently could be too costly, so
I'm not sure we should.

> In my second approach, I have tried to
> than copying from primary). 

Yeah, and I think we could create a dedicated field with this information
if we feel the need.

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 26 Mar 2024 21:59:23 +0530

On Tue, Mar 26, 2024 at 4:35 PM Bharath Rupireddy
<[email-masked]> wrote:
> to the current time so that it gets its own time.

I'm attaching v24 patches. It implements the above idea proposed
upthread for synced slots. I've now separated
s/last_inactive_time/inactive_since and synced slots behaviour. Please
have a look.

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 26 Mar 2024 17:52:12 +0000

Hi,

Thanks!

==== v24-0001

It's now pure mechanical changes and it looks good to me.

==== v24-0002

1 ===

    This commit does two things:
    1) Updates inactive_since for sync slots with the value
    received from the primary's slot.

Tested it and it does that.

2 ===

    2) Ensures the value is set to current timestamp during the
    shutdown of slot sync machinery to help correctly interpret the
    time if the standby gets promoted without a restart.

Tested it and it does that.

3 ===

+/*
+ * Reset the synced slots info such as inactive_since after shutting
+ * down the slot sync machinery.
+ */
+static void
+update_synced_slots_inactive_time(void)

Looks like the comment "reset" is not matching the name of the function and
what it does.

4 ===

+                       /*
+                        * We get the current time beforehand and only once to avoid
+                        * system calls overhead while holding the lock.
+                        */
+                       if (now == 0)
+                               now = GetCurrentTimestamp();

Also +1 of having GetCurrentTimestamp() just called one time within the loop.

5 ===

-               if (!(RecoveryInProgress() && slot->data.synced))
+               if (!(InRecovery && slot->data.synced))
                        slot->inactive_since = GetCurrentTimestamp();
                else
                        slot->inactive_since = 0;

Not related to this change but more the way RestoreSlotFromDisk() behaves here:

For a sync slot on standby it will be set to zero and then later will be
synchronized with the one coming from the primary. I think that's fine to have
it to zero for this window of time.

Now, if the standby is down and one sets sync_replication_slots to off,
then inactive_since will be set to zero on the standby at startup and not 
synchronized (unless one triggers a manual sync). I also think that's fine but
it might be worth to document this behavior (that after a standby startup
inactive_since is zero until the next sync...). 

6 ===

+       print "HI  $slot_name $name $inactive_since $slot_creation_time\n";

garbage?

7 ===

+# Capture and validate inactive_since of a given slot.
+sub capture_and_validate_slot_inactive_since
+{
+       my ($node, $slot_name, $slot_creation_time) = @_;
+       my $name = $node->name;

We know have capture_and_validate_slot_inactive_since at 2 places:
040_standby_failover_slots_sync.pl and 019_replslot_limit.pl.

Worth to create a sub in Cluster.pm?

Regards,

---

From: shveta malik <[email-masked]>
Date: Wed, 27 Mar 2024 09:01:50 +0530

> s/last_inactive_time/inactive_since and synced slots behaviour. Please

Thanks for the patches. Few trivial comments for v24-002:

1)
slot.c:
+ * data from the remote slot. We use InRecovery flag instead of
+ * RecoveryInProgress() as it always returns true even for normal
+ * server startup.

a) Not clear what 'it' refers to. Better to use 'the latter'
b) Is it better to mention the primary here:
 'as the latter always returns true even on the primary server during startup'.

2)
update_local_synced_slot():

- strcmp(remote_slot->plugin, NameStr(slot->data.plugin)) == 0)
+ strcmp(remote_slot->plugin, NameStr(slot->data.plugin)) == 0 &&
+ remote_slot->inactive_since == slot->inactive_since)

When this code was written initially, the intent was to do strcmp at
the end (only if absolutely needed). It will be good if we maintain
the same and add new checks before strcmp.

3)
update_synced_slots_inactive_time():

This assert is removed, is it intentional?
Assert(s->active_pid == 0);

4)
040_standby_failover_slots_sync.pl:

+# Capture the inactive_since of the slot from the standby the logical failover
+# slots are synced/created on the standby.

The comment is unclear, something seems missing.

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 27 Mar 2024 10:08:33 +0530

On Tue, Mar 26, 2024 at 11:22 PM Bertrand Drouvot
<[email-masked]> wrote:
> > upthread for synced slots.
>
> ==== v24-0002
>
> 1 ===
>

Thanks. I've added a test case for this.

> Tested it and it does that.

Thanks. I've added a test case for this.

> Looks like the comment "reset" is not matching the name of the function and

Changed. I've also changed the function name to
update_synced_slots_inactive_since to be precise on what it exactly
does.

Right.

Right.

> inactive_since is zero until the next sync...).

Isn't this behaviour applicable for other slot parameters that the
slot syncs from the remote slot on the primary?

I've added the following note in the comments when we update
inactive_since in RestoreSlotFromDisk.

         * Note that for synced slots after the standby starts up (i.e. after
         * the slots are loaded from the disk), the inactive_since will remain
         * zero until the next slot sync cycle.
         */
        if (!(InRecovery && slot->data.synced))
            slot->inactive_since = GetCurrentTimestamp();
        else
            slot->inactive_since = 0;

> +       print "HI  $slot_name $name $inactive_since $slot_creation_time\n";

Removed.

> Worth to create a sub in Cluster.pm?

I'd second that thought for now. We might have to debate first if it's
useful for all the nodes even without replication, and if yes, the
naming stuff and all that. Historically, we've had such duplicated
functions until recently, for instance advance_wal and log_contains.
We
moved them over to a common perl library Cluster.pm very recently. I'm
sure we can come back later to move it to Cluster.pm.

>  'as the latter always returns true even on the primary server during startup'.

Modified.

> the same and add new checks before strcmp.

Done.

> Assert(s->active_pid == 0);

Yes, the slot can get acquired in the corner case when someone runs
pg_sync_replication_slots concurrently at this time. I'm referring to
the issue reported upthread. We don't prevent one running
pg_sync_replication_slots in promotion/ShutDownSlotSync phase right?
Maybe we should prevent that otherwise some of the slots are synced
and the standby gets promoted while others are yet-to-be-synced.

> The comment is unclear, something seems missing.

Nice catch. Yes, that was wrong. I've modified it now.

Please find the attached v25-0001 (made this 0001 patch now as
inactive_since patch is committed) patch with the above changes.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 Mar 2024 10:22:30 +0530

> and the standby gets promoted while others are yet-to-be-synced.

We should do something about it but that shouldn't be done in this
patch. We can handle it separately and then add such an assert.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 27 Mar 2024 10:24:32 +0530

> patch. We can handle it separately and then add such an assert.

Agreed. Once this patch is concluded, I can fix the slot sync shutdown
issue and will also add this 'assert' back.

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 27 Mar 2024 10:26:01 +0530

> issue and will also add this 'assert' back.

Agreed. Thanks.

---

From: shveta malik <[email-masked]>
Date: Wed, 27 Mar 2024 10:55:29 +0530

> I think that calling GetCurrentTimestamp() so frequently could be too costly, so

Agreed.

> Yeah, and I think we could create a dedicated field with this information

Okay.

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 27 Mar 2024 11:05:04 +0530

> inactive_since patch is committed) patch with the above changes.

Fixed an issue in synchronize_slots where DatumGetLSN is being used in
place of DatumGetTimestampTz. Found this via CF bot member [1], not on
my dev system.

Please find the attached v6 patch.

[1]
[05:14:39.281] #7  DatumGetLSN (X=<optimized out>) at
../src/include/utils/pg_lsn.h:24
[05:14:39.281] No locals.
[05:14:39.281] #8  synchronize_slots (wrconn=wrconn@entry=0x583cd170)
at ../src/backend/replication/logical/slotsync.c:757
[05:14:39.281]         isnull = false
[05:14:39.281]         remote_slot = 0x583ce1a8
[05:14:39.281]         d = <optimized out>
[05:14:39.281]         col = 10
[05:14:39.281]         slotRow = {25, 25, 3220, 3220, 28, 16, 16, 25, 25, 1184}
[05:14:39.281]         res = 0x583cd1b8
[05:14:39.281]         tupslot = 0x583ce11c
[05:14:39.281]         remote_slot_list = 0x0
[05:14:39.281]         some_slot_updated = false
[05:14:39.281]         started_tx = false
[05:14:39.281]         query = 0x57692bc4 "SELECT slot_name, plugin,
confirmed_flush_lsn, restart_lsn, catalog_xmin, two_phase, failover,
database, invalidation_reason, inactive_since FROM
pg_catalog.pg_replication_slots WHERE failover and NOT"...
[05:14:39.281]         __func__ = "synchronize_slots"
[05:14:39.281] #9  0x56ff9d1e in SyncReplicationSlots
(wrconn=0x583cd170) at
../src/backend/replication/logical/slotsync.c:1504

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 27 Mar 2024 05:48:46 +0000

Hi,

> slot syncs from the remote slot on the primary?

No they are persisted on disk. If not, we'd not know where to resume the decoding
from on the standby in case primary is down and/or sync is off.

>             slot->inactive_since = 0;

I think we should add some words in the doc too and also about what the meaning
of inactive_since on the standby is (as suggested by Shveta in [1]).

[1]: https://www.postgresql.org/message-id/CAJpy0uDkTW%2Bt1k3oPkaipFBzZePfFNB5DmiA%3D%3DpxRGcAdpF%3DPg%40mail.gmail.com

> sure we can come back later to move it to Cluster.pm.

I thought that would be the right time not to introduce duplicated code.

Regards,

---

From: shveta malik <[email-masked]>
Date: Wed, 27 Mar 2024 11:39:04 +0530

> Please find the attached v6 patch.

Thanks for the patch. Few trivial things:

----------
1)
system-views.sgml:

a) "Note that the slots" --> "Note that the slots on the standbys,"
--it is good to mention "standbys" as synced could be true on primary
as well (promoted standby)

b) If you plan to add more info which Bertrand suggested, then it will
be better to make a <note> section instead of using "Note"

2)
commit msg:

"The impact of this
on a promoted standby inactive_since is always NULL for all
synced slots even after server restart.
"
Sentence looks broken.
---------

Apart from the above trivial things, v26-001 looks good to me.

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 27 Mar 2024 14:55:17 +0530

> Thanks for the patch. Few trivial things:

Thanks for reviewing.

Done.

> be better to make a <note> section instead of using "Note"

I added the note that Bertrand specified upthread. But, I couldn't
find an instance of adding <note> ... </note> within a table. Hence
with "Note that ...." statments just like any other notes in the
system-views.sgml. pg_replication_slot in system-vews.sgml renders as
table, so having <note> ... </note> may not be a great idea.

> synced slots even after server restart.

Reworded.

> Apart from the above trivial things, v26-001 looks good to me.

Please check the attached v27 patch which also has Bertrand's comment
on deduplicating the TAP function. I've now moved it to Cluster.pm.

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 27 Mar 2024 10:11:58 +0000

Hi,

Thanks!

1 ===

+        Note that the slots on the standbys that are being synced from a
+        primary server (whose <structfield>synced</structfield> field is
+        <literal>true</literal>), will get the
+        <structfield>inactive_since</structfield> value from the
+        corresponding remote slot on the primary. Also, note that for the
+        synced slots on the standby, after the standby starts up (i.e. after
+        the slots are loaded from the disk), the inactive_since will remain
+        zero until the next slot sync cycle.

Not sure we should mention the "(i.e. after the slots are loaded from the disk)"
and also "cycle" (as that does not sound right in case of manual sync).

My proposal (in text) but feel free to reword it:

Note that the slots on the standbys that are being synced from a
primary server (whose synced field is true), will get the inactive_since value
from the corresponding remote slot on the primary. Also, after the standby starts
up, the inactive_since (for such synced slots) will remain zero until the next
synchronization.

2 ===

+=item $node->create_logical_slot_on_standby(self, primary, slot_name, dbname)

get_slot_inactive_since_value instead?

3 ===

+against given reference time.

s/given reference/optional given reference/?

Apart from the above, LGTM.

Regards,

---

From: shveta malik <[email-masked]>
Date: Wed, 27 Mar 2024 15:43:22 +0530

> on deduplicating the TAP function. I've now moved it to Cluster.pm.

Thanks for the patch. Regarding doc, I have few comments.

a)  "inactive_since will remain  zero"
Since it is user exposed info and the user finds it NULL in
pg_replication_slots, shall we mention NULL instead of 0?

b) Since we are referring to the sync cycle here, I feel it will be
good to give a link to that page.
+        zero until the next slot sync cycle (see
+        <xref linkend="logicaldecoding-replication-slots-synchronization"/> for
+        slot synchronization details).

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 27 Mar 2024 17:55:05 +0530

> up, the inactive_since (for such synced slots) will remain zero until the next

WFM.

> get_slot_inactive_since_value instead?

Ugh. Changed.

Done.

Thanks for reviewing.

Thanks for reviewing.

> pg_replication_slots, shall we mention NULL instead of 0?

Right. Changed.

> +        slot synchronization details).

WFM.

Please see the attached v28 patch.

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 27 Mar 2024 13:24:52 +0000

Hi,

Thanks!

1 === sorry I missed it in the previous review

        if (!(RecoveryInProgress() && slot->data.synced))
+       {
                now = GetCurrentTimestamp();
+               update_inactive_since = true;
+       }
+       else
+               update_inactive_since = false;

I think update_inactive_since is not needed, we could rely on (now > 0) instead.

2 ===

+=item $node->get_slot_inactive_since_value(self, primary, slot_name, dbname)
+
+Get inactive_since column value for a given replication slot validating it
+against optional reference time.
+
+=cut
+
+sub get_slot_inactive_since_value
+{

shouldn't be "=item $node->get_slot_inactive_since_value(self, slot_name, reference_time)"
instead?

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 27 Mar 2024 21:00:37 +0530

> I think update_inactive_since is not needed, we could rely on (now > 0) instead.

Thought of using it, but, at the expense of readability. I prefer to
use a variable instead. However, I changed the variable to be more
meaningful to is_slot_being_synced.

Ugh. Changed.

> Apart from the above, LGTM.

Thanks. I'm attaching v29 patches. 0001 managing inactive_since on the
standby for sync slots. 0002 implementing inactive timeout GUC based
invalidation mechanism.

Please have a look.

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 27 Mar 2024 16:12:23 +0000

Hi,

On Wed, Mar 27, 2024 at 09:00:37PM +0530, Bharath Rupireddy wrote:
> use a variable instead.

That's fine too.

> However, I changed the variable to be more meaningful to is_slot_being_synced.

Yeah makes sense and even easier to read.

v29-0001 LGTM.

Regards,

---

From: shveta malik <[email-masked]>
Date: Thu, 28 Mar 2024 09:16:17 +0530

> standby for sync slots. 0002 implementing inactive timeout GUC based

Thanks for the patches. v29-001 looks good to me.

thanks
Shveta

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 28 Mar 2024 09:43:44 +0000

Hi,

Thanks!

Regarding 0002:

Some testing:

T1 ===

When the slot is invalidated on the primary, then the reason is propagated to
the sync slot (if any). That's fine but we are loosing the inactive_since on the
standby:

Primary:

postgres=# select slot_name,inactive_since,conflicting,invalidation_reason from pg_replication_slots where slot_name='lsub29_slot';
  slot_name  |        inactive_since         | conflicting | invalidation_reason
-------------+-------------------------------+-------------+---------------------
 lsub29_slot | 2024-03-28 08:24:51.672528+00 | f           | inactive_timeout
(1 row)

Standby:

postgres=# select slot_name,inactive_since,conflicting,invalidation_reason from pg_replication_slots where slot_name='lsub29_slot';
  slot_name  | inactive_since | conflicting | invalidation_reason
-------------+----------------+-------------+---------------------
 lsub29_slot |                | f           | inactive_timeout
(1 row)

I think in this case it should always reflect the value from the primary (so
that one can understand why it is invalidated).

T2 ===

And it is set to a value during promotion:

postgres=# select pg_promote();
 pg_promote
------------
 t
(1 row)

postgres=# select slot_name,inactive_since,conflicting,invalidation_reason from pg_replication_slots where slot_name='lsub29_slot';
  slot_name  |        inactive_since        | conflicting | invalidation_reason
-------------+------------------------------+-------------+---------------------
 lsub29_slot | 2024-03-28 08:30:11.74505+00 | f           | inactive_timeout
(1 row)

I think when it is invalidated it should always reflect the value from the
primary (so that one can understand why it is invalidated).

T3 ===

As far the slot invalidation on the primary:

postgres=# SELECT * FROM pg_logical_slot_get_changes('lsub29_slot', NULL, NULL, 'include-xids', '0');
ERROR:  cannot acquire invalidated replication slot "lsub29_slot"

Can we make the message more consistent with what can be found in CreateDecodingContext()
for example?

T4 ===

Also, it looks like querying pg_replication_slots() does not trigger an
invalidation: I think it should if the slot is not invalidated yet (and matches
the invalidation criteria).

Code review:

CR1 ===

+        Invalidate replication slots that are inactive for longer than this
+        amount of time. If this value is specified without units, it is taken

s/Invalidate/Invalidates/?

Should we mention the relationship with inactive_since?

CR2 ===

+ *
+ * If check_for_invalidation is true, the slot is checked for invalidation
+ * based on replication_slot_inactive_timeout GUC and an error is raised after making the slot ours.
  */
 void
-ReplicationSlotAcquire(const char *name, bool nowait)
+ReplicationSlotAcquire(const char *name, bool nowait,
+                                          bool check_for_invalidation)

s/check_for_invalidation/check_for_timeout_invalidation/?

CR3 ===

+       if (slot->inactive_since == 0 ||
+               replication_slot_inactive_timeout == 0)
+               return false;

Better to test replication_slot_inactive_timeout first? (I mean there is no
point of testing inactive_since if replication_slot_inactive_timeout == 0)

CR4 ===

+       if (slot->inactive_since > 0 &&
+               replication_slot_inactive_timeout > 0)
+       {

Same.

So, instead of CR3 === and CR4 ===, I wonder if it wouldn't be better to do
something like:

if (replication_slot_inactive_timeout == 0)
	return false;
else if (slot->inactive_since > 0)
.
.
.
.
else
	return false;

That would avoid checking replication_slot_inactive_timeout and inactive_since
multiple times.

CR5 ===

+        * held to avoid race conditions -- for example the restart_lsn could move
+        * forward, or the slot could be dropped.

Does the restart_lsn example makes sense here?

CR6 ===

+static bool
+InvalidateSlotForInactiveTimeout(ReplicationSlot *slot, bool need_locks)
+{

InvalidatePossiblyInactiveSlot() maybe?

CR7 ===

+       /* Make sure the invalidated state persists across server restart */
+       slot->just_dirtied = true;
+       slot->dirty = true;
+       SpinLockRelease(&slot->mutex);

Maybe we could create a new function say MarkGivenReplicationSlotDirty()
with a slot as parameter, that ReplicationSlotMarkDirty could call too?

Then maybe we could set slot->data.invalidated = RS_INVAL_INACTIVE_TIMEOUT in
InvalidateSlotForInactiveTimeout()? (to avoid multiple SpinLockAcquire/SpinLockRelease).

CR8 ===

+       if (persist_state)
+       {
+               char            path[MAXPGPATH];
+
+               sprintf(path, "pg_replslot/%s", NameStr(slot->data.name));
+               SaveSlotToPath(slot, path, ERROR);
+       }

Maybe we could create a new function say GivenReplicationSlotSave()
with a slot as parameter, that ReplicationSlotSave() could call too?

CR9 ===

+       if (check_for_invalidation)
+       {
+               /* The slot is ours by now */
+               Assert(s->active_pid == MyProcPid);
+
+               /*
+                * Well, the slot is not yet ours really unless we check for the
+                * invalidation below.
+                */
+               s->active_pid = 0;
+               if (InvalidateReplicationSlotForInactiveTimeout(s, true, true))
+               {
+                       /*
+                        * If the slot has been invalidated, recalculate the resource
+                        * limits.
+                        */
+                       ReplicationSlotsComputeRequiredXmin(false);
+                       ReplicationSlotsComputeRequiredLSN();
+
+                       /* Might need it for slot clean up on error, so restore it */
+                       s->active_pid = MyProcPid;
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                                        errmsg("cannot acquire invalidated replication slot \"%s\"",
+                                                       NameStr(MyReplicationSlot->data.name))));
+               }
+               s->active_pid = MyProcPid;

Are we not missing some SpinLockAcquire/Release on the slot's mutex here? (the
places where we set the active_pid).

CR10 ===

@@ -1628,6 +1674,10 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
                                        if (SlotIsLogical(s))
                                                invalidation_cause = cause;
                                        break;
+                               case RS_INVAL_INACTIVE_TIMEOUT:
+                                       if (InvalidateReplicationSlotForInactiveTimeout(s, false, false))
+                                               invalidation_cause = cause;
+                                       break;

InvalidatePossiblyObsoleteSlot() is not called with such a reason, better to use
an Assert here and in the caller too?

CR11 ===

+++ b/src/test/recovery/t/050_invalidate_slots.pl

why not using 019_replslot_limit.pl?

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Fri, 29 Mar 2024 09:39:31 +0530

On Wed, Mar 27, 2024 at 9:00 PM Bharath Rupireddy
<[email-masked]> wrote:
> standby for sync slots.

Commit message states: "why we can't just update inactive_since for
synced slots on the standby with the value received from remote slot
on the primary. This is consistent with any other slot parameter i.e.
all of them are synced from the primary."

The inactive_since is not consistent with other slot parameters which
we copy. We don't perform anything related to those other parameters
like say two_phase phase which can change that property. However, we
do acquire the slot, advance the slot (as per recent discussion [1]),
and release it. Since these operations can impact inactive_since, it
seems to me that inactive_since is not the same as other parameters.
It can have a different value than the primary. Why would anyone want
to know the value of inactive_since from primary after the standby is
promoted? Now, the other concern is that calling GetCurrentTimestamp()
could be costly when the values for the slot are not going to be
updated but if that happens we can optimize such that before acquiring
the slot we can have some minimal pre-checks to ensure whether we need
to update the slot or not.

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

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 29 Mar 2024 06:19:05 +0000

Hi,

On Fri, Mar 29, 2024 at 09:39:31AM +0530, Amit Kapila wrote:
> promoted?

I think it can be useful "before" it is promoted and in case the primary is down.
I agree that tracking the activity time of a synced slot can be useful, why
not creating a dedicated field for that purpose (and keep inactive_since a
perfect "copy" of the primary)?

> Now, the other concern is that calling GetCurrentTimestamp()

Right, but for a very active slot it is likely that we call GetCurrentTimestamp()
during almost each sync cycle.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Fri, 29 Mar 2024 15:03:01 +0530

> I think it can be useful "before" it is promoted and in case the primary is down.

It is not clear to me what is user going to do by checking the
inactivity time for slots when the corresponding server is down. I
thought the idea was to check such slots and see if they need to be
dropped or enabled again to avoid excessive disk usage, etc.

> perfect "copy" of the primary)?

We can have a separate field for this but not sure if it is worth it.

> during almost each sync cycle.

True, but if we have to save a slot to disk each time to persist the
changes (for an active slot) then probably GetCurrentTimestamp()
shouldn't be costly enough to matter.

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Sun, 31 Mar 2024 10:25:46 +0530

> Regarding 0002:

Thanks for reviewing it.

>   slot_name  |        inactive_since         | conflicting | invalidation_reason

I'll come back to this as soon as we all agree on inactive_since
behavior for synced slots.

> postgres=# select pg_promote();

I'll come back to this as soon as we all agree on inactive_since
behavior for synced slots.

> Can we make the message more consistent with what can be found in CreateDecodingContext()

Hm, that makes sense because slot acquisition and release is something
internal to the server.

> the invalidation criteria).

There's a different opinion on this, check comment #3 from
https://www.postgresql.org/message-id/CAA4eK1LLj%2BeaMN-K8oeOjfG%2BUuzTY%3DL5PXbcMJURZbFm%2B_aJSA%40mail.gmail.com.

Done.

Done.

Done.

Done.

> Does the restart_lsn example makes sense here?

No, it doesn't. Modified that.

> InvalidatePossiblyInactiveSlot() maybe?

I think we will lose the essence i.e. timeout from the suggested
function name, otherwise just the inactive doesn't give a clearer
meaning. I kept it that way unless anyone suggests otherwise.

> with a slot as parameter, that ReplicationSlotMarkDirty could call too?

Done that.

> InvalidateSlotForInactiveTimeout()? (to avoid multiple SpinLockAcquire/SpinLockRelease).

Done that.

> with a slot as parameter, that ReplicationSlotSave() could call too?

Done that.

> places where we set the active_pid).

Hm, yes. But, shall I acquire the mutex, set active_pid to 0 for a
moment just to satisfy Assert(slot->active_pid == 0); in
InvalidateReplicationSlotForInactiveTimeout and
InvalidateSlotForInactiveTimeout? I just removed the assertions
because being replication_slot_inactive_timeout > 0 and inactive_since
> 0 is enough for these functions to think and decide on inactive
timeout invalidation.

Done.

> why not using 019_replslot_limit.pl?

I understand that 019_replslot_limit covers wal_removed related
invalidations. But, I don't want to kludge it with a bunch of other
tests. The new tests anyway need a bunch of new nodes and a couple of
helper functions. Any future invalidation mechanisms can be added here
in this new file. Also, having a separate file quickly helps isolate
any test failures that BF animals might report in future. I don't
think a separate test file here hurts anyone unless there's a strong
reason against it.

Please see the attached v30 patch. 0002 is where all of the above
review comments have been addressed.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 1 Apr 2024 08:47:59 +0530

> to know the value of inactive_since from primary after the standby is

After thinking about it for a while now, it feels to me that the
synced slots (slots on the standby that are being synced from the
primary) can have their own inactive_sicne value. Fundamentally,
inactive_sicne is set to 0 when slot is acquired and set to current
time when slot is released, no matter who acquires and releases it -
be it walsenders for replication, or backends for slot advance, or
backends for slot sync using pg_sync_replication_slots, or backends
for other slot functions, or background sync worker. Remember the
earlier patch was updating inactive_since just for walsenders, but
then the suggestion was to update it unconditionally -
https://www.postgresql.org/message-id/CAJpy0uD64X%3D2ENmbHaRiWTKeQawr-rbGoy_GdhQQLVXzUSKTMg%40mail.gmail.com.
Whoever syncs the slot, *acutally* acquires the slot i.e. makes it
theirs, syncs it from the primary, and releases it. IMO, no
differentiation is to be made for synced slots.

There was a suggestion on using inactive_since of the synced slot on
the standby to know the inactivity of the slot on the primary. If one
wants to do that, they better look at/monitor the primary slot
info/logs/pg_replication_slot/whatever. I really don't see a point in
having two different meanings for a single property of a replication
slot - inactive_since for a regular slot tells since when this slot
has become inactive, and for a synced slot since when the
corresponding remote slot has become inactive. I think this will
confuse users for sure.

Also, if inactive_since is being changed on the primary so frequently,
and none of the other parameters are changing, if we copy
inactive_since to the synced slots, then standby will just be doing
*sync* work (mark the slots dirty and save to disk) for updating
inactive_since. I think this is unnecessary behaviour for sure.

Coming to a future patch for inactive timeout based slot invalidation,
we can either allow invalidation without any differentiation for
synced slots or restrict invalidation to avoid more sync work. For
instance, if inactive timeout is kept low on the standby, the sync
worker will be doing more work as it drops and recreates a slot
repeatedly if it keeps getting invalidated. Another thing is that the
standby takes independent invalidation decisions for synced slots.
AFAICS, invalidation due to wal_removal is the only sole reason (out
of all available invalidation reasons) for a synced slot to get
invalidated independently of the primary. Check
https://www.postgresql.org/message-id/CAA4eK1JXBwTaDRD_%3D8t6UB1fhRNjC1C%2BgH4YdDxj_9U6djLnXw%40mail.gmail.com
for the suggestion on we better not differentiaing invalidation
decisions for synced slots.

The assumption of letting synced slots have their own inactive_since
not only simplifies the code, but also looks less-confusing and more
meaningful to the user. The only code that we put in on top of the
committed code is to use InRecovery in place of
RecoveryInProgress() in RestoreSlotFromDisk() to fix the issue raised
by Shveta upthread.

> [1] - https://www.postgresql.org/message-id/OS0PR01MB571615D35F486080616CA841943A2%40OS0PR01MB5716.jpnprd01.prod.outlook.com

A quick test with a function to measure the cost of
GetCurrentTimestamp [1] on my Ubuntu dev system (an AWS EC2 c5.4xlarge
instance), gives me [2]. It took 0.388 ms, 2.269 ms, 21.144 ms,
209.333 ms, 2091.174 ms, 20908.942 ms for 10K, 100K, 1million,
10million, 100million, 1billion times respectively. Costs might be
different on various systems with different OS, but it gives us a
rough idea.

If we are too much concerned about the cost of GetCurrentTimestamp(),
a possible approach is just don't set inactive_since for slots being
synced on the standby. Just let the first acquisition and release
after the promotion do that job. We can always call this out in the
docs saying "replication slots on the streaming standbys which are
being synced from the primary are not inactive in practice, so the
inactive_since is always NULL for them unless the standby is
promoted".

[1]
Datum
pg_get_current_timestamp(PG_FUNCTION_ARGS)
{
    int         loops = PG_GETARG_INT32(0);
    TimestampTz ctime;

    for (int i = 0; i < loops; i++)
        ctime = GetCurrentTimestamp();

    PG_RETURN_TIMESTAMPTZ(ctime);
}

[2]
postgres=# \timing
Timing is on.
postgres=# SELECT pg_get_current_timestamp(1000000000);
   pg_get_current_timestamp
-------------------------------
 2024-03-30 19:07:57.374797+00
(1 row)

Time: 20908.942 ms (00:20.909)
postgres=# SELECT pg_get_current_timestamp(100000000);
   pg_get_current_timestamp
-------------------------------
 2024-03-30 19:08:21.038064+00
(1 row)

Time: 2091.174 ms (00:02.091)
postgres=# SELECT pg_get_current_timestamp(10000000);
   pg_get_current_timestamp
-------------------------------
 2024-03-30 19:08:24.329949+00
(1 row)

Time: 209.333 ms
postgres=# SELECT pg_get_current_timestamp(1000000);
   pg_get_current_timestamp
-------------------------------
 2024-03-30 19:08:26.978016+00
(1 row)

Time: 21.144 ms
postgres=# SELECT pg_get_current_timestamp(100000);
   pg_get_current_timestamp
-------------------------------
 2024-03-30 19:08:29.142248+00
(1 row)

Time: 2.269 ms
postgres=# SELECT pg_get_current_timestamp(10000);
   pg_get_current_timestamp
------------------------------
 2024-03-30 19:08:31.34621+00
(1 row)

Time: 0.388 ms

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 1 Apr 2024 07:18:55 +0000

Hi,

On Mon, Apr 01, 2024 at 08:47:59AM +0530, Bharath Rupireddy wrote:
> info/logs/pg_replication_slot/whatever.

Yeah but the use case was in case the primary is down for whatever reason.

> I really don't see a point in

I'm not sure as we are speaking about "synced" slots. I can also see some confusion
if this value is not "synced".

> inactive_since. I think this is unnecessary behaviour for sure.

Right, I think we should avoid the save slot to disk in that case (question raised
by Amit in [1]).

> decisions for synced slots.

Yeah, I think the invalidation decision on the standby is highly linked to
what inactive_since on the standby is: synced from primary or not.

> meaningful to the user.

I'm not sure at all. But if the majority of us thinks it's the case then let's
go that way.

> > to update the slot or not.

Also maybe we could accept a bit less accuracy and use
GetCurrentTransactionStopTimestamp() instead?

> synced on the standby.
> Just let the first acquisition and release

I think that was the initial behavior that lead to Robert's remark (see [2]):

"
And I'm suspicious that having an exception for slots being synced is
a bad idea. That makes too much of a judgement about how the user will
use this field. It's usually better to just expose the data, and if
the user needs helps to make sense of that data, then give them that
help separately.
"

[1]: https://www.postgresql.org/message-id/CAA4eK1JtKieWMivbswYg5FVVB5FugCftLvQKVsxh%3Dm_8nk04vw%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CA%2BTgmob_Ta-t2ty8QrKHBGnNLrf4ZYcwhGHGFsuUoFrAEDw4sA%40mail.gmail.com

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 1 Apr 2024 09:59:51 +0000

Hi,

> behavior for synced slots.

Makes sense. Also if the majority of us thinks it's not needed for inactive_since
to be an exact copy of the primary, then let's go that way.

> behavior for synced slots.

Yeah.

> https://www.postgresql.org/message-id/CAA4eK1LLj%2BeaMN-K8oeOjfG%2BUuzTY%3DL5PXbcMJURZbFm%2B_aJSA%40mail.gmail.com.

Oh right, I can see Amit's point too. Let's put pg_replication_slots() out of
the game then.

> meaning. I kept it that way unless anyone suggests otherwise.

Right. OTOH I think that "Possibly" adds some nuance (like InvalidatePossiblyObsoleteSlot()
is already doing).

> review comments have been addressed.

Thanks! FYI, I did not look at the content yet, just replied to the above
comments.

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 2 Apr 2024 12:07:54 +0900

> differentiation is to be made for synced slots.

FWIW, coming to this thread late, I think that the inactive_since
should not be synchronized from the primary. The wall clocks are
different on the primary and the standby so having the primary's
timestamp on the standby can confuse users, especially when there is a
big clock drift. Also, as Amit mentioned, inactive_since seems not to
be consistent with other parameters we copy. The
replication_slot_inactive_timeout feature should work on the standby
independent from the primary, like other slot invalidation mechanisms,
and it should be based on its own local clock.

> RecoveryInProgress() in RestoreSlotFromDisk() to fix the issue raised

If we want to invalidate the synced slots due to the timeout, I think
we need to define what is "inactive" for synced slots.

Suppose that the slotsync worker updates the local (synced) slot's
inactive_since whenever releasing the slot, irrespective of the actual
LSNs (or other slot parameters) having been updated. I think that this
idea cannot handle a slot that is not acquired on the primary. In this
case, the remote slot is inactive but the local slot is regarded as
active.  WAL files are piled up on the standby (and on the primary) as
the slot's LSNs don't move forward. I think we want to regard such a
slot as "inactive" also on the standby and invalidate it because of
the timeout.

> > to update the slot or not.

If we use such pre-checks, another problem might happen; it cannot
handle a case where the slot is acquired on the primary but its LSNs
don't move forward. Imagine a logical replication conflict happened on
the subscriber, and the logical replication enters the retry loop. In
this case, the remote slot's inactive_since gets updated for every
retry, but it looks inactive from the standby since the slot LSNs
don't change. Therefore, only the local slot could be invalidated due
to the timeout but probably we don't want to regard such a slot as
"inactive".

Another idea I came up with is that the slotsync worker updates the
local slot's inactive_since to the local timestamp only when the
remote slot might have got inactive. If the remote slot is acquired by
someone, the local slot's inactive_since is also NULL. If the remote
slot gets inactive, the slotsync worker sets the local timestamp to
the local slot's inactive_since. Since the remote slot could be
acquired and released before the slotsync worker gets the remote slot
data again, if the remote slot's inactive_since > the local slot's
inactive_since, the slotsync worker updates the local one. IOW, we
detect whether the remote slot was acquired and released since the
last synchronization, by checking the remote slot's inactive_since.
This idea seems to handle these cases I mentioned unless I'm missing
something, but it requires for the slotsync worker to update
inactive_since in a different way than other parameters.

Or a simple solution is that the slotsync worker updates
inactive_since as it does for non-synced slots, and disables
timeout-based slot invalidation for synced slots.

Regards,

-- 
Masahiko Sawada

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 2 Apr 2024 06:28:40 +0000

Hi,

> and it should be based on its own local clock.

Thanks for sharing your thoughts! So, it looks like that most of us agree to not
sync inactive_since from the primary, I'm fine with that.

> slot as "inactive" also on the standby and invalidate it because of

I think that makes sense to somehow link inactive_since on the standby to 
the actual LSNs (or other slot parameters) being updated or not.

> inactive_since, the slotsync worker updates the local one.

Then I think we would need to be careful about time zone comparison.

> IOW, we

Yeah, I think the main question to help us decide is: do we want to invalidate
"inactive" synced slots locally (in addition to synchronizing the invalidation
from the primary)? 

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 2 Apr 2024 12:41:35 +0530

> "inactive" synced slots locally (in addition to synchronizing the invalidation

I think this approach looks way simpler than the other one. The other
approach of linking inactive_since on the standby for synced slots to
the actual LSNs (or other slot parameters) being updated or not looks
more complicated, and might not go well with the end user.  However,
we need to be able to say why we don't invalidate synced slots due to
inactive timeout unlike the wal_removed invalidation that can happen
right now on the standby for synced slots. This leads us to define
actually what a slot being active means. Is syncing the data from the
remote slot considered as the slot being active?

On the other hand, it may not sound great if we don't invalidate
synced slots due to inactive timeout even though they hold resources
such as WAL and XIDs.

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 2 Apr 2024 08:33:40 +0000

Hi,

> synced slots due to inactive timeout even though they hold resources

Right and the "only" benefit then would be to give an idea as to when the last
sync did occur on the local slot.

Regards,

---

From: shveta malik <[email-masked]>
Date: Wed, 3 Apr 2024 08:38:10 +0530

> sync inactive_since from the primary, I'm fine with that.

+1 on not syncing slots from primary.

> Then I think we would need to be careful about time zone comparison.

Yes. Also we need to consider the case when a user is relying on
pg_sync_replication_slots() and has not enabled slot-sync worker. In
such a case if synced slot's inactive_since is derived from inactivity
of remote-slot, it might not be that frequently updated (based on when
the user actually runs the SQL function) and thus may be misleading.
OTOH, if inactivty_since of synced slots represents its own
inactivity, then it will give correct info even for the case when the
SQL function is run after a long time and slot-sync worker is
disabled.

> > timeout-based slot invalidation for synced slots.

I like this idea better, it takes care of such a case too when the
user is relying on sync-function rather than worker and does not want
to get the slots invalidated in between 2 sync function calls.

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 3 Apr 2024 11:17:41 +0530

> to get the slots invalidated in between 2 sync function calls.

Please find the attached v31 patches implementing the above idea:

- synced slots get their on inactive_since just like any other slot
- 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)
- synced slots inactive_since is set to current timestamp after the
standby gets promoted to help inactive_since interpret correctly just
like any other slot.

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 3 Apr 2024 08:17:05 +0000

Hi,

Thanks!

Some comments regarding v31-0002:

=== testing the behavior

T1 ===

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

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?

=== code

CR1 ===

+        Invalidates replication slots that are inactive for longer the
+        specified amount of time

s/for longer the/for longer that/?

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.

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"?

CR4 ===

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

Same.

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?

CR6 ===

+       if (replication_slot_inactive_timeout == 0)
+               return false;
+       else if (slot->inactive_since > 0)

"else" is not needed here.

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.

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/?

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?

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

Regards,

---

From: shveta malik <[email-masked]>
Date: Wed, 3 Apr 2024 14:57:55 +0530

> Please find the attached v31 patches implementing the above idea:

Thanks for the patches, please find few comments:

v31-001:

1)
system-views.sgml:
value will get updated  after every synchronization from the
corresponding remote slot on the primary.

--This is confusing. It will be good to rephrase it.

2)
update_synced_slots_inactive_since()

--May be, we should mention in the header that this function is called
only during promotion.

3) 040_standby_failover_slots_sync.pl:
We capture inactive_since_on_primary when we do this for the first time at #175
ALTER SUBSCRIPTION regress_mysub1 DISABLE"

But we again recreate the sub and disable it at line #280.
Do you think we shall get inactive_since_on_primary again here, to be
compared with inactive_since_on_new_primary later?

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

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?

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.

<have not reviewed 002 patch in detail yet>

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Wed, 3 Apr 2024 16:19:46 +0530

> compared with inactive_since_on_new_primary later?

I think so.

Few additional comments on tests:
1.
+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;"

Shall we do <= check as we are doing in the main function
get_slot_inactive_since_value as the time duration is less so it can
be the same as well? Similarly, please check other tests.

2.
+=item $node->get_slot_inactive_since_value(self, slot_name, reference_time)
+
+Get inactive_since column value for a given replication slot validating it
+against optional reference time.
+
+=cut
+
+sub get_slot_inactive_since_value

I see that all callers validate against reference time. It is better
to name it validate_slot_inactive_since rather than using get_* as the
main purpose is to validate the passed value.

-- 
With Regards,
Amit Kapila.