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

On Mon, Nov 18, 2024 at 12:24 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> 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.
>

Looks reasonable to me.

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

If I recall correctly, we have kept the current name after much
discussion. I don't want to get into this discussion again unless we
have a strong reason for the same.

--
With Regards,
Amit Kapila.