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.