Re: persist logical slots to disk during shutdown checkpoint

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.

When to detect that divergence and what to do when there is divergence
should be document at relevant places in the code. In future if we
expand the When and How we use this variable, the comment in
ReplicationSlot will be insufficient.

We follow this commenting style at several places e.g.
/* any outstanding modifications? */
bool just_dirtied;
bool dirty;

how and when these variables are used is commented upon in the relevant code.

  * This needn't actually be part of a checkpoint, but it's a convenient
- * location.
+ * location. is_shutdown is true in case of a shutdown checkpoint.

Relying on the first sentence, if we decide to not persist the
replication slot at the time of checkpoint, would that be OK? It
doesn't look like a convenience thing to me any more.

--
Best Wishes,
Ashutosh Bapat