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. ~~~~ 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
v1-0001-Clarify-description-of-inactive_since-field.patch
Description: Binary data
