On Thu, Sep 7, 2023 at 3:38 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Sep 7, 2023 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Sep 7, 2023 at 1:18 PM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > On Thu, Sep 07, 2023 at 11:56:28AM +0530, Amit Kapila wrote:
> > > > Thanks, the patch looks good to me as well.
> > >
> > > + /* This is used to track the last saved confirmed_flush LSN value */
> > > + XLogRecPtr last_saved_confirmed_flush;
> > >
> > > This does not feel sufficient, as the comment explaining what this
> > > variable does uses the same terms as the variable name (aka it is the
> > > last save of the confirmed_lsn). Why it it here and why it is useful?
> > > In which context and/or code paths is it used? Okay, there are some
> > > explanations when saving a slot, restoring a slot or when a checkpoint
> > > processes slots, but it seems important to me to document more things
> > > in ReplicationSlot where this is defined.
> > >
> >
> > Hmm, this is quite debatable as different people feel differently
> > about this. The patch author kept it where it is now but in one of my
> > revisions, I rewrote and added it in the ReplicationSlot. Then
> > Ashutosh argued that it is better to keep it near where we are saving
> > the slot (aka where the patch has) [1]. Anyway, as I also preferred
> > the core part of the theory about this variable to be in
> > ReplicationSlot, so I'll move it there before commit unless someone
> > argues against it or has any other comments.
>
> If we want it to be in ReplicationSlot, I suggest we just say, - saves
> last confirmed flush LSN to detect any divergence in the in-memory and
> on-disk confirmed flush LSN cheaply.
>
I have added something on these lines and also changed the other
comment pointed out by you. In the passing, I made minor cosmetic
changes as well.
--
With Regards,
Amit Kapila.