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