Thread: Re: Avoid updating inactive_since for invalid replication slots

Hi Nisha,

Some review comments for v2-0001.

======
doc/src/sgml/system-views.sgml

1.
The time when the slot became inactive. NULL if the slot is currently
being streamed. If the slot becomes invalid, this value will never be
updated. Note that for slots on the standby that are being synced from
a primary server (whose synced field is true), the inactive_since
indicates the time when slot synchronization (see Section 47.2.3) was
most recently stopped. NULL if the slot has always been synchronized.
On standby, this is useful for slots that are being synced from a
primary server (whose synced field is true) so they know when the slot
stopped being synchronized.

~

(maybe not strictly related to this patch, but perhaps you can fix it
in passing because it will help the readability of the newly added
sentence also...)

There are 2 different explanations for NULL:
"NULL if the slot is currently being streamed."
"NULL if the slot has always been synchronized."

I'm assuming that 2nd description is only to be read in the scope of
"Note that for slots on the standby that are being synced from a
primary server...". IMO inserting a blank line before "Note that for
slots on the standby..." will help separate these two quite different
descriptions for the same field.

~~~

Apart from the above comment the v2 patch looked ok to me.

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

On Wed, Feb 5, 2025 at 2:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Feb 5, 2025 at 5:53 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Some review comments for v2-0001.
> >
> > ======
> > doc/src/sgml/system-views.sgml
> >
> > 1.
> > The time when the slot became inactive. NULL if the slot is currently
> > being streamed. If the slot becomes invalid, this value will never be
> > updated. Note that for slots on the standby that are being synced from
> > a primary server (whose synced field is true), the inactive_since
> > indicates the time when slot synchronization (see Section 47.2.3) was
> > most recently stopped. NULL if the slot has always been synchronized.
> > On standby, this is useful for slots that are being synced from a
> > primary server (whose synced field is true) so they know when the slot
> > stopped being synchronized.
> >
> > ~
> >
> > (maybe not strictly related to this patch, but perhaps you can fix it
> > in passing because it will help the readability of the newly added
> > sentence also...)
> >
> > There are 2 different explanations for NULL:
> > "NULL if the slot is currently being streamed."
> > "NULL if the slot has always been synchronized."
> >
> > I'm assuming that 2nd description is only to be read in the scope of
> > "Note that for slots on the standby that are being synced from a
> > primary server...". IMO inserting a blank line before "Note that for
> > slots on the standby..." will help separate these two quite different
> > descriptions for the same field.
> >
>
> This is unrelated to this patch, but I don't mind you proposing a
> separate patch if you feel it will make it clear. Did you see separate
> paragraphs in other descriptions?
>

OK, I have started a new thread [1] for this.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPssvVMTWVtUPto6HbPO8pgVsvtzndt_FdBomA_Oq4zf3w%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia