Re: Incorrect logic in XLogNeedsFlush()

On Wed, Sep 10, 2025 at 12:48:21PM +0530, Dilip Kumar wrote:
>> So, looking at the code, it seems like most places we examine
>> ControlFile->minRecoveryPoint, we condition it on "InArchiveRecovery".
>> Is this something we want to do in XLogNeedsFlush() to avoid reading
>> from ControlFile->minRecoveryPoint()?

How would that help when the checkpointer does XLogNeedsFlush() calls,
which is where you are reporting the disturbance is during the
end-of-recovery checkpoint?  InArchiveRecovery is only true in the
startup process, set when we are doing archive recovery.  This can be
switched from the beginning of recovery or later, once all the local
WAL has been replayed.

>> Though, it seems like LocalMinRecoveryPoint must be getting
>> incorrectly set elsewhere, otherwise this would have guarded us from
>> examining the control file:
>>
>>         if (XLogRecPtrIsInvalid(LocalMinRecoveryPoint) && InRecovery)
>>             updateMinRecoveryPoint = false;
>>
>>         /* Quick exit if already known to be updated or cannot be updated */
>>         if (record <= LocalMinRecoveryPoint || !updateMinRecoveryPoint)
>>             return false;
>
> That's not quite right. Before the end-of-recovery checkpoint, the
> InRecovery flag is already set to false. This means that even if
> LocalMinRecoveryPoint is invalid, it won't matter, and
> updateMinRecoveryPoint will not be set to false. Since
> LocalMinRecoveryPoint is 0, the condition if (record <=
> LocalMinRecoveryPoint) will also fail, causing the process to continue
> and read from the ControlFile.

Similarly, InRecovery can only be true in the startup process.

In the case of 015_promotion_pages, I can see that when the check you
are adding fails, the LSN is already flushed by XLogFLush().  So,
while I don't see any active bug here, I tend to agree with your
position that XLogNeedsFlush() could be improved in the context of the
end-of-recovery checkpoint or just when a process is allowed WAL
inserts while we are still in recovery.

You have a good point here, especially knowing that for
CHECKPOINT_END_OF_RECOVERY case in CreateCheckPoint(), called by the
checkpointer, we allow WAL inserts with LocalSetXLogInsertAllowed().
So, relying on RecoveryInProgress() in this context leads to a wrong
result with XLogNeedsFlush(): WAL inserts are allowed, the minimum
recovery point is not relevant for the evaluation.

Or are you worried about the case of GetVictimBuffer() where a second
call of XLogNeedsFlush() lives?
--
Michael