On Mon, Nov 18, 2024 at 12:24 PM Nisha Moond <nisha.moond...@gmail.com> wrote: > > On Fri, Nov 15, 2024 at 3:01 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Thu, Nov 14, 2024 at 12:12 PM Nisha Moond <nisha.moond...@gmail.com> > > wrote: > > > > > > On Thu, Oct 31, 2024 at 11:05 PM Bruce Momjian <br...@momjian.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.
