Re: Incorrect logic in XLogNeedsFlush()

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.

>> /*
>> @@ -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().
--
Michael