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.

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 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]/messages/by-id/CAHut+PvSQWG-m6wUfoQfnShTXDAjoa3-MhNQ=_N3GBM31g1Vbw@mail.gmail.com, 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]/messages/by-id/CAHut+PvSQWG-m6wUfoQfnShTXDAjoa3-MhNQ=_N3GBM31g1Vbw@mail.gmail.com:

- 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]/messages/by-id/ZyO_2NWC5MyWq0bH@momjian.us 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]: /messages/by-id/CAHut+PvSQWG-m6wUfoQfnShTXDAjoa3-MhNQ=_N3GBM31g1Vbw@mail.gmail.com
[2]: /messages/by-id/ZyO_2NWC5MyWq0bH@momjian.us

--
Thanks,
Nisha

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.