Thread: Re: DOCS - pg_replication_slot . Fix the 'inactive_since' description
On Mon, Sep 2, 2024 at 5:47 AM Peter Smith <smithpb2250@gmail.com> wrote: > > Hi hackers. While reviewing another thread I had cause to look at the > docs for the pg_replication_slot 'inactive_since' field [1] for the > first time. > > I was confused by the description, which is as follows: > ---- > inactive_since timestamptz > The time since the slot has become inactive. NULL if the slot is > currently being used. > ---- > > Then I found the github history for the patch [2], and the > accompanying long thread discussion [3] about the renaming of that > field. I have no intention to re-open that can-of-worms, but OTOH I > feel the first sentence of the field description is wrong and needs > fixing. > > Specifically, IMO describing something as "The time since..." means > some amount of elapsed time since some occurrence, but that is not the > correct description for this timestamp field. > > This is not just a case of me being pedantic. For example, here is > what Chat-GPT had to say: > ---- > I asked: > What does "The time since the slot has become inactive." mean? > > ChatGPT said: > "The time since the slot has become inactive" refers to the duration > that has passed from the moment a specific slot (likely a database > replication slot or a similar entity) stopped being active. In other > words, it measures how much time has elapsed since the slot > transitioned from an active state to an inactive state. > > For example, if a slot became inactive 2 hours ago, "the time since > the slot has become inactive" would be 2 hours. > ---- > > To summarize, the current description wrongly describes the field as a > time duration: > "The time since the slot has become inactive." > > I suggest replacing it with: > "The slot has been inactive since this time." > +1 for the change. If I had read the document without knowing about the patch, I too would have interpreted it as a duration. thanks Shveta
On Mon, Sep 2, 2024 at 9:14 AM shveta malik <shveta.malik@gmail.com> wrote: > > On Mon, Sep 2, 2024 at 5:47 AM Peter Smith <smithpb2250@gmail.com> wrote: > > ---- > > > > To summarize, the current description wrongly describes the field as a > > time duration: > > "The time since the slot has become inactive." > > > > I suggest replacing it with: > > "The slot has been inactive since this time." > > > > +1 for the change. If I had read the document without knowing about > the patch, I too would have interpreted it as a duration. > The suggested change looks good to me as well. I'll wait for a day or two before pushing to see if anyone thinks otherwise. -- With Regards, Amit Kapila.
On Tue, Sep 3, 2024 at 10:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > >
> > > To summarize, the current description wrongly describes the field as a
> > > time duration:
> > > "The time since the slot has become inactive."
> > >
> > > I suggest replacing it with:
> > > "The slot has been inactive since this time."
> > >
> >
> > +1 for the change. If I had read the document without knowing about
> > the patch, I too would have interpreted it as a duration.
> >
>
> The suggested change looks good to me as well. I'll wait for a day or
> two before pushing to see if anyone thinks otherwise.
Shall we make the change in code-comment as well:
typedef struct ReplicationSlot
{
...
/* The time since the slot has become inactive */
TimestampTz inactive_since;
}
thanks
Shveta
Hi, On Tue, Sep 03, 2024 at 10:43:14AM +0530, Amit Kapila wrote: > On Mon, Sep 2, 2024 at 9:14 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Mon, Sep 2, 2024 at 5:47 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > ---- > > > > > > To summarize, the current description wrongly describes the field as a > > > time duration: > > > "The time since the slot has become inactive." > > > > > > I suggest replacing it with: > > > "The slot has been inactive since this time." > > > > > > > +1 for the change. If I had read the document without knowing about > > the patch, I too would have interpreted it as a duration. > > > > The suggested change looks good to me as well. I'll wait for a day or > two before pushing to see if anyone thinks otherwise. I'm not 100% convinced the current wording is confusing because: - the field type is described as a "timestamptz". - there is no duration unit in the wording (if we were to describe a duration, we would probably add an unit to it, like "The time (in s)..."). That said, if we want to highlight that this is not a duration, what about? "The time (not duration) since the slot has become inactive." Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Tue, Sep 3, 2024 at 4:35 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Tue, Sep 03, 2024 at 10:43:14AM +0530, Amit Kapila wrote: > > On Mon, Sep 2, 2024 at 9:14 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > On Mon, Sep 2, 2024 at 5:47 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > ---- > > > > > > > > To summarize, the current description wrongly describes the field as a > > > > time duration: > > > > "The time since the slot has become inactive." > > > > > > > > I suggest replacing it with: > > > > "The slot has been inactive since this time." > > > > > > > > > > +1 for the change. If I had read the document without knowing about > > > the patch, I too would have interpreted it as a duration. > > > > > > > The suggested change looks good to me as well. I'll wait for a day or > > two before pushing to see if anyone thinks otherwise. > > I'm not 100% convinced the current wording is confusing because: > > - the field type is described as a "timestamptz". > - there is no duration unit in the wording (if we were to describe a duration, > we would probably add an unit to it, like "The time (in s)..."). > Hmm. I assure you it is confusing because in English "The time since" implies duration, and that makes the sentence contrary to the timestamptz field type. Indeed, I cited the Chat-GPT's interpretation above specifically so that people would not just take this as my opinion. > That said, if we want to highlight that this is not a duration, what about? > > "The time (not duration) since the slot has become inactive." > There is no need to "highlight" anything. To avoid confusion we only need to say what we mean. ====== Kind Regards, Peter Smith. Fujitsu Australia
Hi, On Tue, Sep 03, 2024 at 05:52:53PM +1000, Peter Smith wrote: > On Tue, Sep 3, 2024 at 4:35 PM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > Hi, > > > > On Tue, Sep 03, 2024 at 10:43:14AM +0530, Amit Kapila wrote: > > > On Mon, Sep 2, 2024 at 9:14 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > > > > > On Mon, Sep 2, 2024 at 5:47 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > ---- > > > > > > > > > > To summarize, the current description wrongly describes the field as a > > > > > time duration: > > > > > "The time since the slot has become inactive." > > > > > > > > > > I suggest replacing it with: > > > > > "The slot has been inactive since this time." > > > > > > > > > > > > > +1 for the change. If I had read the document without knowing about > > > > the patch, I too would have interpreted it as a duration. > > > > > > > > > > The suggested change looks good to me as well. I'll wait for a day or > > > two before pushing to see if anyone thinks otherwise. > > > > I'm not 100% convinced the current wording is confusing because: > > > > - the field type is described as a "timestamptz". > > - there is no duration unit in the wording (if we were to describe a duration, > > we would probably add an unit to it, like "The time (in s)..."). > > > > Hmm. I assure you it is confusing because in English "The time since" > implies duration, and that makes the sentence contrary to the > timestamptz field type. Oh if that implies duration (I'm not a native English speaker and would have assumed that it does not imply a duration 100% of the time) then yeah there is some contradiction between the wording and the returned type. > Indeed, I cited the Chat-GPT's interpretation > above specifically so that people would not just take this as my > opinion. Right, I was just wondering what would Chat-GPT answer if you add "knowing that the time is of timestamptz datatype" to the question? > To avoid confusion we only need to say what we mean. Sure, I was just saying that I did not see any confusion given the returned datatype. Now that you say that "The time since" implies duration then yeah, in that case, it's better to provide the right wording then. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
At Tue, 3 Sep 2024 10:43:14 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in > On Mon, Sep 2, 2024 at 9:14 AM shveta malik <shveta.malik@gmail.com> wrote: > > > > On Mon, Sep 2, 2024 at 5:47 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > ---- > > > > > > To summarize, the current description wrongly describes the field as a > > > time duration: > > > "The time since the slot has become inactive." > > > > > > I suggest replacing it with: > > > "The slot has been inactive since this time." > > > > > > > +1 for the change. If I had read the document without knowing about > > the patch, I too would have interpreted it as a duration. > > > > The suggested change looks good to me as well. I'll wait for a day or > two before pushing to see if anyone thinks otherwise. If possible, I'd prefer to use "the time" as the subject. For example, would "The time this slot was inactivated" be acceptable? However, this loses the sense of continuation up to that point, so if that's crucial, the current proposal might be better. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: DOCS - pg_replication_slot . Fix the 'inactive_since' description
From
"David G. Johnston"
Date:
On Tuesday, September 3, 2024, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
At Tue, 3 Sep 2024 10:43:14 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
> On Mon, Sep 2, 2024 at 9:14 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Mon, Sep 2, 2024 at 5:47 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > ----
> > >
> > > To summarize, the current description wrongly describes the field as a
> > > time duration:
> > > "The time since the slot has become inactive."
> > >
> > > I suggest replacing it with:
> > > "The slot has been inactive since this time."
> > >
> >
> > +1 for the change. If I had read the document without knowing about
> > the patch, I too would have interpreted it as a duration.
> >
>
> The suggested change looks good to me as well. I'll wait for a day or
> two before pushing to see if anyone thinks otherwise.
If possible, I'd prefer to use "the time" as the subject. For example,
would "The time this slot was inactivated" be acceptable? However,
this loses the sense of continuation up to that point, so if that's
crucial, the current proposal might be better.
Agree on sticking with “The time…”
Thus I suggest either:
The time when the slot became inactive.
The time when the slot was deactivated.
Apparently inactivate is a valid choice here but it definitely sounds like unusual usage in this context. Existing usage (via GibHub search…someone may want to grep) seems to support deactivate as well.
I like the first suggestion more, especially since becoming inactive can happen without user input. Saying deactivate could be seen to imply user intervention.
David J.
On Wed, Sep 4, 2024 at 11:34 AM David G. Johnston <david.g.johnston@gmail.com> wrote: > > > Agree on sticking with “The time…” > > Thus I suggest either: > > The time when the slot became inactive. +1. Definitely removes confusion caused by "since" and keeps The time as subject. -- Best Wishes, Ashutosh Bapat
Saying "The time..." is fine, but the suggestions given seem backwards to me: - The time this slot was inactivated - The time when the slot became inactive. - The time when the slot was deactivated. e.g. It is not like light switch. So, there is no moment when the active slot "became inactive" or "was deactivated". Rather, the 'inactive_since' timestamp field is simply: - The time the slot was last active. - The last time the slot was active. ====== Kind Regards, Peter Smith. Fujitsu Australia
Re: DOCS - pg_replication_slot . Fix the 'inactive_since' description
From
"David G. Johnston"
Date:
On Sun, Sep 8, 2024, 18:55 Peter Smith <smithpb2250@gmail.com> wrote:
Saying "The time..." is fine, but the suggestions given seem backwards to me:
- The time this slot was inactivated
- The time when the slot became inactive.
- The time when the slot was deactivated.
e.g. It is not like light switch. So, there is no moment when the
active slot "became inactive" or "was deactivated".
While this is plausible the existing wording and the name of the field definitely fail to convey this.
Rather, the 'inactive_since' timestamp field is simply:
- The time the slot was last active.
- The last time the slot was active.
I see your point but that wording is also quite confusing when an active slot returns null for this field.
At this point I'm confused enough to need whatever wording is taken to be supported by someone explaining the code that interacts with this field.
I suppose I'm expecting something like: The time the last activity finished, or null if an activity is in-progress.
David J.
On Mon, Sep 9, 2024 at 12:20 PM David G. Johnston <david.g.johnston@gmail.com> wrote: > > > > On Sun, Sep 8, 2024, 18:55 Peter Smith <smithpb2250@gmail.com> wrote: >> >> Saying "The time..." is fine, but the suggestions given seem backwards to me: >> - The time this slot was inactivated >> - The time when the slot became inactive. >> - The time when the slot was deactivated. >> >> e.g. It is not like light switch. So, there is no moment when the >> active slot "became inactive" or "was deactivated". > > > While this is plausible the existing wording and the name of the field definitely fail to convey this. > >> >> Rather, the 'inactive_since' timestamp field is simply: >> - The time the slot was last active. >> - The last time the slot was active. > > > I see your point but that wording is also quite confusing when an active slot returns null for this field. > > At this point I'm confused enough to need whatever wording is taken to be supported by someone explaining the code thatinteracts with this field. > Me too. I created this thread primarily to get the description changed to clarify this field represents a moment in time, rather than a duration. So I will be happy with any wording that addresses that. > I suppose I'm expecting something like: The time the last activity finished, or null if an activity is in-progress. ====== Kind Regards, Peter Smith. Fujitsu Australia
On Wed, Oct 16, 2024 at 10:56 PM Bruce Momjian <bruce@momjian.us> wrote:
>
> On Mon, Sep 9, 2024 at 01:15:32PM +1000, Peter Smith wrote:
> >
> > Me too. I created this thread primarily to get the description changed
> > to clarify this field represents a moment in time, rather than a
> > duration. So I will be happy with any wording that addresses that.
>
> I dug into the code and came up with the attached patch. "active" means
> there is a process streaming the slot, and the "inactive_since" time
> means the last time synchronous slot streaming was stopped. Doc patch
> attached.
>
Few comments:
=============
1.
<para>
- True if this slot is currently actively being used
+ True if this slot is currently currently being streamed
</para></entry>
currently shouldn't be used twice.
2.
- /* The time since the slot has become inactive */
+ /* The time slot sychronized was stopped. */
TimestampTz inactive_since;
Would it be better to say: "The time slot synchronization was stopped."?
3.
This is useful for slots on the
+ standby that are intended to be synced from a primary server
I think it is better to be explicit here and probably say: "This is
useful for slots on the standby that are being synced from a primary
server .."
--
With Regards,
Amit Kapila.
Hi. Here are a couple of minor comments. 1. + The time slot synchronization (see <xref + linkend="logicaldecoding-replication-slots-synchronization"/>) + was most recently stopped. /The time slot/The time when slot/ ~~~ 2. - /* The time since the slot has become inactive */ + /* The time slot sychronized was stopped. */ Maybe just make this comment the same as the sentence used in the docs: - e.g. add "when"; fix, typo "sychronized", etc. /The time slot sychronized was stopped./The time when slot synchronization was most recently stopped./ ====== Kind Regards, Peter Smith. Fujitsu Australia
On Thu, Oct 31, 2024 at 11:05 PM Bruce Momjian <bruce@momjian.us> wrote: > > Yes, all good suggestions, updated patch attached. > LGTM. -- With Regards, Amit Kapila.
On Thu, Oct 31, 2024 at 11:05 PM Bruce Momjian <bruce@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. 2) Can we add more details to it for column's behavior on restarting a node, something like - "For the inactive slots, restarting a node resets the "inactive_since" time to the node's start time. " -- Thanks, Nisha
On Thu, Nov 14, 2024 at 12:12 PM Nisha Moond <nisha.moond412@gmail.com> wrote: > > On Thu, Oct 31, 2024 at 11:05 PM Bruce Momjian <bruce@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? > 2) Can we add more details to it for column's behavior on restarting > a node, something like - > "For the inactive slots, restarting a node resets the "inactive_since" > time to the node's start time. " > I am not so sure about this. Adding too-long descriptions also sometimes confuses users. -- With Regards, Amit Kapila.
On Mon, Nov 18, 2024 at 12:24 PM Nisha Moond <nisha.moond412@gmail.com> wrote: > > On Fri, Nov 15, 2024 at 3:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Thu, Nov 14, 2024 at 12:12 PM Nisha Moond <nisha.moond412@gmail.com> wrote: > > > > > > On Thu, Oct 31, 2024 at 11:05 PM Bruce Momjian <bruce@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.
On Mon, Nov 18, 2024 at 01:31:45PM +0530, Amit Kapila wrote: > On Mon, Nov 18, 2024 at 12:24 PM Nisha Moond <nisha.moond412@gmail.com> wrote: > > -- 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. +1 -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com When a patient asks the doctor, "Am I going to die?", he means "Am I going to die soon?"
On Tue, Nov 19, 2024 at 1:26 AM Bruce Momjian <bruce@momjian.us> wrote: > > On Mon, Nov 18, 2024 at 01:31:45PM +0530, Amit Kapila wrote: > > On Mon, Nov 18, 2024 at 12:24 PM Nisha Moond <nisha.moond412@gmail.com> wrote: > > > > > > Attached the patch with modification. > > > > > > > Looks reasonable to me. > > +1 > Pushed. -- With Regards, Amit Kapila.