Avoid updating inactive_since for invalid replication slots

Hi Nisha.

Some review comments for patch v1-0001

======
GENERAL - Missing Test case?

1.
Should there be some before/after test case for 'active_since' value
with invalid slots to verify that the patch is doing what it says?

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

ReplicationSlotAcquire:

2.
I saw some other code in ReplicationSlotAcquire doing:

/*
* Reset the time since the slot has become inactive as the slot is active
* now.
*/
SpinLockAcquire(&s->mutex);
s->inactive_since = 0;
SpinLockRelease(&s->mutex);

~

Should that be changed to use the new function like:
ReplicationSlotSetInactiveSince(s, 0, true);

Or (if not) then it probably needs a comment to say why not.
~~~

RestoreSlotFromDisk:

3.
+ if (now == 0)
+ now = GetCurrentTimestamp();
+
+ ReplicationSlotSetInactiveSince(slot, now, false);

3a.
The code from v65-0001 in the other thread [1]/messages/by-id/CABdArM7D9u1an6gzfArAL32Jn0QQkKs7JffUxcZ9EqzAaGrfvQ@mail.gmail.com (where the bulk of this
v1 patch came from) used to have a RestoreSlotFromDisk function
comment saying "Avoid calling ReplicationSlotSetInactiveSince() here,
as it will not set the time for invalid slots."

In other words, yesterday we were deliberately NOT calling function
ReplicationSlotSetInactiveSince, but today we deliberately ARE calling
it (???).

Why has it changed?

~~~

3b.
The other code that is similar to this deferred assignment of 'now'
(see function update_synced_slots_inactive_since) includes a comment
/* Use the same inactive_since time for all the slots. */.

Should this code do the same?

~

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

4.
+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;
+
+ if (acquire_lock)
+ SpinLockRelease(&s->mutex);
+}
+

4a.
I think it might not be a good idea to call the parameter 'now',
because that might not always be the meaning -- e.g. In another
comment I suggested passing a value 0.

A more generic TimestampTz name like 'ts' might be better here; just
the caller argument can be called 'now' when appropriate.

~

4b.
Since this is a static inline function I assume performance is
important (e.g. sometimes it is called within a spinlock). If that is
the case, then maybe writing this logic with fewer conditions would be
better.

SUGGESTION

if (s->data.invalidated == RS_INVAL_NONE)
{
if (acquire_lock)
{
SpinLockAcquire(&s->mutex);
s->inactive_since = ts;
SpinLockRelease(&s->mutex);
}
else
s->inactive_since = now;
}

======
[1]: /messages/by-id/CABdArM7D9u1an6gzfArAL32Jn0QQkKs7JffUxcZ9EqzAaGrfvQ@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Feb 4, 2025 at 8:37 AM Peter Smith <smithpb2250@gmail.com> wrote:

Some review comments for patch v1-0001

======
GENERAL - Missing Test case?

1.
Should there be some before/after test case for 'active_since' value
with invalid slots to verify that the patch is doing what it says?

I think the existing tests should be sufficient. Adding a new test
just for invalid slots means we need to consider the other properties
of slots as well because in general the invalid slots shouldn't be
updated.

RestoreSlotFromDisk:

3.
+ if (now == 0)
+ now = GetCurrentTimestamp();
+
+ ReplicationSlotSetInactiveSince(slot, now, false);

3a.
The code from v65-0001 in the other thread [1] (where the bulk of this
v1 patch came from) used to have a RestoreSlotFromDisk function
comment saying "Avoid calling ReplicationSlotSetInactiveSince() here,
as it will not set the time for invalid slots."

In other words, yesterday we were deliberately NOT calling function
ReplicationSlotSetInactiveSince, but today we deliberately ARE calling
it (???).

Why has it changed?

See my last comment in that thread (1). In short, the invalid slots
should never be update inactive_since and the same should be updated
in docs.

~~~

3b.
The other code that is similar to this deferred assignment of 'now'
(see function update_synced_slots_inactive_since) includes a comment
/* Use the same inactive_since time for all the slots. */.

Should this code do the same?

This makes sense.

4b.
Since this is a static inline function I assume performance is
important (e.g. sometimes it is called within a spinlock). If that is
the case, then maybe writing this logic with fewer conditions would be
better.

SUGGESTION

if (s->data.invalidated == RS_INVAL_NONE)
{
if (acquire_lock)
{
SpinLockAcquire(&s->mutex);
s->inactive_since = ts;
SpinLockRelease(&s->mutex);
}
else
s->inactive_since = now;
}

I prefer the current coding in the patch as it is simpler to follow.

(1) - /messages/by-id/CAA4eK1L6A-RC2PkqAFjgmmmS6_vKxrLsG33vdJzeeRKBP8RbOA@mail.gmail.com

--
With Regards,
Amit Kapila.

On Monday, February 3, 2025 8:03 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

Hi Hackers,
(CC people involved in the earlier discussion)

Right now, it is possible for the 'inactive_since' value of an invalid replication
slot to be updated multiple times, which is unexpected behavior.
As suggested in the ongoing thread [1], this patch introduces a new dedicated
function to update the inactive_since for a given replication slot, and ensures
that inactive_since is not updated for an invalid replication slot.

The v1 patch is attached. Any feedback would be appreciated.

Thanks for sharing the patch, and I agree we should avoid updating
inactive_since for invalid slots.

But I have one question for the code:

+/*
+ * 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;

I am wondering is it safe to access the 'invalidated' flag without taking spinlock ?
Strictly speaking, I think it's only safe to access slot property without lock
when the slot is acquiring by the current process. So, if it's safe here,
could you please add some comments to clarify the same ?

Best Regards,
Hou zj

On Tue, Feb 4, 2025 at 9:33 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Monday, February 3, 2025 8:03 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

Hi Hackers,
(CC people involved in the earlier discussion)

Right now, it is possible for the 'inactive_since' value of an invalid replication
slot to be updated multiple times, which is unexpected behavior.
As suggested in the ongoing thread [1], this patch introduces a new dedicated
function to update the inactive_since for a given replication slot, and ensures
that inactive_since is not updated for an invalid replication slot.

The v1 patch is attached. Any feedback would be appreciated.

Thanks for sharing the patch, and I agree we should avoid updating
inactive_since for invalid slots.

But I have one question for the code:

+/*
+ * 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;

I am wondering is it safe to access the 'invalidated' flag without taking spinlock ?
Strictly speaking, I think it's only safe to access slot property without lock
when the slot is acquiring by the current process. So, if it's safe here,
could you please add some comments to clarify the same ?

Agree, the invalidated flag check should be under the spinlock when
the process doesn't own the slot.
Here is the v2 patch with above change and other comments from [1] and
[2]: incorporated.

--
Thanks,
Nisha

On Tue, 4 Feb 2025 at 11:52, Nisha Moond <nisha.moond412@gmail.com> wrote:

Here is the v2 patch with above change and other comments from [1] and
[2] incorporated.

One small suggestion:
Since we will not be retaining inactive time for invalid slots after
server restart, the inactive time will be lost in this case, shouldn't
we include mentioning that too:
         The time when the slot became inactive. <literal>NULL</literal> if the
-        slot is currently being streamed.
+        slot is currently being streamed. If the slot becomes invalid,
+        this value will not be updated.

Regards,
Vignesh

On Tue, Feb 4, 2025 at 12:05 PM vignesh C <vignesh21@gmail.com> wrote:

On Tue, 4 Feb 2025 at 11:52, Nisha Moond <nisha.moond412@gmail.com> wrote:

Here is the v2 patch with above change and other comments from [1] and
[2] incorporated.

One small suggestion:
Since we will not be retaining inactive time for invalid slots after
server restart, the inactive time will be lost in this case, shouldn't
we include mentioning that too:
The time when the slot became inactive. <literal>NULL</literal> if the
-        slot is currently being streamed.
+        slot is currently being streamed. If the slot becomes invalid,
+        this value will not be updated.

The current update seems sufficient even for restart cases. It
indicates that inactive_sinsce is not updated for invalid slots
whether its restart or otherwise.

--
With Regards,
Amit Kapila.

Hi Nisha,

Some review comments for v2-0001.

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

1.
The time when the slot became inactive. NULL if the slot is currently
being streamed. If the slot becomes invalid, this value will never be
updated. 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 time when slot synchronization (see Section 47.2.3) was
most recently stopped. NULL if the slot has always been synchronized.
On standby, this is useful for slots that are being synced from a
primary server (whose synced field is true) so they know when the slot
stopped being synchronized.

~

(maybe not strictly related to this patch, but perhaps you can fix it
in passing because it will help the readability of the newly added
sentence also...)

There are 2 different explanations for NULL:
"NULL if the slot is currently being streamed."
"NULL if the slot has always been synchronized."

I'm assuming that 2nd description is only to be read in the scope of
"Note that for slots on the standby that are being synced from a
primary server...". IMO inserting a blank line before "Note that for
slots on the standby..." will help separate these two quite different
descriptions for the same field.

~~~

Apart from the above comment the v2 patch looked ok to me.

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

On Wed, Feb 5, 2025 at 5:53 AM Peter Smith <smithpb2250@gmail.com> wrote:

Some review comments for v2-0001.

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

1.
The time when the slot became inactive. NULL if the slot is currently
being streamed. If the slot becomes invalid, this value will never be
updated. 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 time when slot synchronization (see Section 47.2.3) was
most recently stopped. NULL if the slot has always been synchronized.
On standby, this is useful for slots that are being synced from a
primary server (whose synced field is true) so they know when the slot
stopped being synchronized.

~

(maybe not strictly related to this patch, but perhaps you can fix it
in passing because it will help the readability of the newly added
sentence also...)

There are 2 different explanations for NULL:
"NULL if the slot is currently being streamed."
"NULL if the slot has always been synchronized."

I'm assuming that 2nd description is only to be read in the scope of
"Note that for slots on the standby that are being synced from a
primary server...". IMO inserting a blank line before "Note that for
slots on the standby..." will help separate these two quite different
descriptions for the same field.

This is unrelated to this patch, but I don't mind you proposing a
separate patch if you feel it will make it clear. Did you see separate
paragraphs in other descriptions?

--
With Regards,
Amit Kapila.

On Wed, Feb 5, 2025 at 2:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Feb 5, 2025 at 5:53 AM Peter Smith <smithpb2250@gmail.com> wrote:

Some review comments for v2-0001.

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

1.
The time when the slot became inactive. NULL if the slot is currently
being streamed. If the slot becomes invalid, this value will never be
updated. 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 time when slot synchronization (see Section 47.2.3) was
most recently stopped. NULL if the slot has always been synchronized.
On standby, this is useful for slots that are being synced from a
primary server (whose synced field is true) so they know when the slot
stopped being synchronized.

~

(maybe not strictly related to this patch, but perhaps you can fix it
in passing because it will help the readability of the newly added
sentence also...)

There are 2 different explanations for NULL:
"NULL if the slot is currently being streamed."
"NULL if the slot has always been synchronized."

I'm assuming that 2nd description is only to be read in the scope of
"Note that for slots on the standby that are being synced from a
primary server...". IMO inserting a blank line before "Note that for
slots on the standby..." will help separate these two quite different
descriptions for the same field.

This is unrelated to this patch, but I don't mind you proposing a
separate patch if you feel it will make it clear. Did you see separate
paragraphs in other descriptions?

OK, I have started a new thread [1]/messages/by-id/CAHut+PssvVMTWVtUPto6HbPO8pgVsvtzndt_FdBomA_Oq4zf3w@mail.gmail.com for this.

======
[1]: /messages/by-id/CAHut+PssvVMTWVtUPto6HbPO8pgVsvtzndt_FdBomA_Oq4zf3w@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia