Re: DOCS - pg_replication_slot . Fix the 'inactive_since' description

On Fri, Nov 15, 2024 at 3:01 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Nov 14, 2024 at 12:12 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> >
> > On Thu, Oct 31, 2024 at 11:05 PM Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > >
> > >
> > > Yes, all good suggestions, updated patch attached.
> > >
> >
> > Few comments for the changes under "inactive_since" description:
> >
> > + The time when slot synchronization (see <xref
> > + linkend="logicaldecoding-replication-slots-synchronization"/>)
> > + was most recently stopped. <literal>NULL</literal> if the slot
> > + has always been synchronized. This is useful for slots on the
> > + standby that are being synced from a primary server (whose
> > + <structfield>synced</structfield> field is <literal>true</literal>)
> > + so they know when the slot stopped being synchronized.
> >
> > 1)
> > To me, the above lines give the impression that "inactive_since" is
> > only meaningful for the logical slots which are being synchronized
> > from primary to standby, which is not correct. On a primary node, this
> > column gives the timestamp when any slot becomes inactive. By removing
> > the line -
> > - The time since the slot has become inactive.
> > I feel we lost the description that explains this column’s purpose on
> > primary nodes. I suggest explicitly clarifying the inactive_since
> > column's meaning on primary nodes as well.
> >
>
> Good point. The same holds true for following change in the patch as well:
> - /* The time since the slot has become inactive */
> + /* The time when slot synchronization was most recently stopped. */
> TimestampTz inactive_since;
>
> Do you have suggestions for improving the proposal?
>

Considering earlier discussion in [1], I dig up some details about
when and where the inactive_since field is set to a non-null value:

1) When a slot is released, it is marked as inactive, and immediately
the inactive_since field is set to current time (e.g., when the
respective subscription is disabled, dropped, or the slot is
invalidated).
2) When slots are restored from disk (e.g., during a server restart),
the current time is set as inactive_since for all slots.
3) On a standby server, the slot-sync worker sets the current time (as
"last synchronized time") for all slots being synced from the primary
during each sync cycle.

-- inactive_since will be reset to NULL by the process that acquires
the slot, making it active or in-use again.

AFAIU, inactive_since indicates the point in time when the slot became
inactive, as the field is set immediately when any of the above
conditions are triggered. It is not a case where a periodic process
observes the slot as inactive and sets inactive_since to the "observed
time", even if the slot was deactivated some time ago.

Given this understanding, and to avoid unnecessary complexity, I agree
with David's suggestion [1]:

- The time when the slot became inactive. (retained this in patch as
wording aligns with the field name)
- The time when the slot was deactivated.

Alternatively, we could use "timestamp" instead of "time" to clearly
indicate that this refers to a specific timestamp and not a duration:
"The timestamp indicating when the slot became inactive."

Thoughts?

For the description of synced slots on standby, I’m fine with keeping
Bruce's suggestion from patch [2] as it is.

Attached the patch with modification.
~~~~

On another note, It seems the confusion appears to arise from the
field name, "inactive_since". I believe it would be clearer if the
name were changed to something more descriptive, such as:
- deactivated_at
- became_inactive_at
However, I’m unsure if such a change would be permissible now.

[1] https://www.postgresql.org/message-id/CAHut%2BPvSQWG-m6wUfoQfnShTXDAjoa3-MhNQ%3D_N3GBM31g1Vbw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/ZyO_2NWC5MyWq0bH%40momjian.us

--
Thanks,
Nisha