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

On Tue, Sep 3, 2024 at 4:12 PM shveta malik <shveta.malik@gmail.com> wrote:

...

Shall we make the change in code-comment as well:

typedef struct ReplicationSlot
{
...
/* The time since the slot has become inactive */
TimestampTz inactive_since;
}

Hi Shveta,

Yes, I think so. I hadn't bothered to include this in the v1 patch
only because the docs are user-facing, but this code comment isn't.
However, now that you've mentioned it, I made the same change here
also. Thanks. See patch v2.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

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

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

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 that interacts 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 Mon, Sep 9, 2024 at 01:15:32PM +1000, Peter Smith wrote:

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 that interacts 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 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.

I am not sure what other things are needed, but this is certainly
unclear. This comment from src/backend/replication/logical/slotsync.c
helped me understand this:

* We need to update inactive_since only when we are promoting standby to
* correctly interpret the inactive_since if the standby gets promoted
* without a restart. We don't want the slots to appear inactive for a
* long time after promotion if they haven't been synchronized recently.
* Whoever acquires the slot, i.e., makes the slot active, will reset it.

--
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 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 Fri, Oct 18, 2024 at 04:25:33PM +0530, Amit Kapila wrote:

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

Agreed, fixed in the attached patch.

--
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 Thu, Oct 24, 2024 at 08:32:54AM +1100, Peter Smith wrote:

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

Yes, all good suggestions, updated patch attached.

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