Re: Incorrect logic in XLogNeedsFlush()

On Wed, Sep 24, 2025 at 05:33:08PM -0400, Melanie Plageman wrote:
> I think he's referring to the order of checks inside
> UpdateMinRecoveryPoint(). Something like the attached patch.

Yep, thanks. What you are doing with XLogNeedsFlush() looks correct.

> And, if I'm not mistaken, there was another idea mentioned in [1]
> which was to move to using GetRecoveryState() in XLogNeedsFlush() and
> UpdateMinRecoveryPoint instead of relying on the variable InRecovery +
> XLogRecPtrIsInvalid(LocalMinRecoveryPoint) to distinguish crash
> recovery.

The removal of InRecovery means that we would disable a bit more
aggressively updateMinRecoveryPoint for other processes than the
startup process as InRecovery is only set in the startup process,
which should be OK. Some comments of XLogNeedsFlush(), like the
comment block on top of the new GetRecoveryState() call, would need a
refresh.

XLogNeedsFlush() has a bit further down your change a path where
updateMinRecoveryPoint is set to false for other processes than the
startup process. A shortcut based on GetRecoveryState() should make
its removal possible, simplifying a bit more the code.

> These should probably be in the same commit. I think that if what I
> have is correct, it is a clarity improvement.

I'd rather tackle each thing separately. We're changing slightly
different things here: depending on GetRecoveryState() is one thing a
bit more invasive than the second thing, which is to switch the order
of the checks of updateMinRecoveryPoint. Perhaps I'm the only one
picky here as the lines are thin, though :o)
--
Michael