Re: 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] (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] https://www.postgresql.org/message-id/CABdArM7D9u1an6gzfArAL32Jn0QQkKs7JffUxcZ9EqzAaGrfvQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia