Re: Introduce XID age and inactive timeout based replication slot invalidation

On Wed, Mar 27, 2024 at 2:55 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, Mar 27, 2024 at 11:39 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > Thanks for the patch. Few trivial things:
>
> Thanks for reviewing.
>
> > ----------
> > 1)
> > system-views.sgml:
> >
> > a) "Note that the slots" --> "Note that the slots on the standbys,"
> > --it is good to mention "standbys" as synced could be true on primary
> > as well (promoted standby)
>
> Done.
>
> > b) If you plan to add more info which Bertrand suggested, then it will
> > be better to make a <note> section instead of using "Note"
>
> I added the note that Bertrand specified upthread. But, I couldn't
> find an instance of adding <note> ... </note> within a table. Hence
> with "Note that ...." statments just like any other notes in the
> system-views.sgml. pg_replication_slot in system-vews.sgml renders as
> table, so having <note> ... </note> may not be a great idea.
>
> > 2)
> > commit msg:
> >
> > "The impact of this
> > on a promoted standby inactive_since is always NULL for all
> > synced slots even after server restart.
> > "
> > Sentence looks broken.
> > ---------
>
> Reworded.
>
> > Apart from the above trivial things, v26-001 looks good to me.
>
> Please check the attached v27 patch which also has Bertrand's comment
> on deduplicating the TAP function. I've now moved it to Cluster.pm.
>

Thanks for the patch. Regarding doc, I have few comments.

+ Note that the slots on the standbys that are being synced from a
+ primary server (whose <structfield>synced</structfield> field is
+ <literal>true</literal>), will get the
+ <structfield>inactive_since</structfield> value from the
+ corresponding remote slot on the primary. Also, note that for the
+ synced slots on the standby, after the standby starts up (i.e. after
+ the slots are loaded from the disk), the inactive_since will remain
+ zero until the next slot sync cycle.

a) "inactive_since will remain zero"
Since it is user exposed info and the user finds it NULL in
pg_replication_slots, shall we mention NULL instead of 0?

b) Since we are referring to the sync cycle here, I feel it will be
good to give a link to that page.
+ zero until the next slot sync cycle (see
+ <xref linkend="logicaldecoding-replication-slots-synchronization"/> for
+ slot synchronization details).

thanks
Shveta