Re: Incorrect logic in XLogNeedsFlush()

On Thu, Sep 18, 2025 at 9:24 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Sep 17, 2025 at 09:23:05PM -0400, Melanie Plageman wrote:
> > In terms of comments, I think it is best to update the comment above
> > XLogNeedsFlush(). Something like :
> >
> >  /*
> > - * Test whether XLOG data has been flushed up to (at least) the given position.
> > + * Test whether XLOG data has been flushed up to (at least) the given position
> > + * or whether the minimum recovery point is updated past the given position.
>
> Okay.
>
> > - * Returns true if a flush is still needed.  (It may be that someone else
> > - * is already in process of flushing that far, however.)
> > + * Returns true if a flush is still needed or if the minimum recovery point
> > + * must be updated.
>
> Okay with that as well.  Why not if you find that confusing..
>
> > + * It is possible that someone else is already in the process of flushing that
> > + * far or updating the minimum recovery point that far.
>
> I am not sure about the value this part brings, considering the
> addition with the two other paragraphs.

I think this comment is a side note which is stating that it is
possible that while XLogNeedFlush() is deciding that based on the
current flush position or min recovery point parallely someone might
flush beyond that point.  And it was existing comment which has been
improved by adding min recovery points, so I think it makes sense.

> >>  /*
> >> @@ -3114,8 +3121,13 @@ XLogNeedsFlush(XLogRecPtr record)
> >>       * During recovery, we don't flush WAL but update minRecoveryPoint
> >>       * instead. So "needs flush" is taken to mean whether minRecoveryPoint
> >>       * would need to be updated.
> >> +     *
> >> +     * XLogInsertAllowed() is used as an end-of-recovery checkpoint is
> >> +     * launched while recovery is still in progress, RecoveryInProgress()
> >> +     * returning true in this case.  This check should be consistent with the
> >> +     * one in XLogFlush().
> >>       */
> >
> > I can't quite parse the wording of the above comment.
>
> Yep.  I am missing an "if" here for "is used as, if an
> end-of-recovery", but I am coming with simpler, like:
> Using XLogInsertAllowed() rather than RecoveryInProgress() matters for
> the case of an end-of-recovery checkpoint, where WAL data is flushed.
> This check should be consistent with the one in XLogFlush().

I tried improving this comment as well. Feel free to disregard it if
you think it's not improving it.

--
Regards,
Dilip Kumar
Google