On Thu, Sep 7, 2023 at 3:38 PM Ashutosh Bapat <ashutosh.bapat....@gmail.com> wrote: > > On Thu, Sep 7, 2023 at 1:43 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Thu, Sep 7, 2023 at 1:18 PM Michael Paquier <mich...@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.
v10-0001-Persist-logical-slots-to-disk-during-a-shutdown-.patch
Description: Binary data