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.
[1] - https://www.postgresql.org/message-id/CAExHW5uXq_CU80XJtmWbPJinRjJx54kbQJ9DT%3DUFySKXpjVwrw%40mail.gmail.com
--
With Regards,
Amit Kapila.