Re: Incorrect logic in XLogNeedsFlush()

On Fri, 2025-09-12 at 14:04 +0900, Michael Paquier wrote:
> On Fri, Sep 12, 2025 at 10:21:27AM +0530, Dilip Kumar wrote:
> > Yeah, asserting it at the end makes sense, as we can ensure that
> > XLogFlush() and XLogNeedsFlush() agree on the same thing without
> > adding additional overhead.  Here is the revised one.
>
> Melanie and Jeff, what do you think?

IIUC: during the end-of-recovery checkpoint, a hypothetical call to
XLogNeedsFlush() would have incorrectly used the LocalMinRecoveryPoint;
and with the change, it would correctly use the flush pointer.

That wasn't a live bug because XLogNeedsFlush() was never actually
called during the end-of-recovery checkpoint. CreateCheckPoint()
called XLogFlush() directly, which correctly checked the flush pointer.
But, hypothetically, if that code had logic based around
XLogNeedsFlush() before calling XLogFlush(), that would have been a
bug.

So this fix has no behavior change, but it would make the code more
resilient against changes to CreateCheckPoint(), more consistent, and
less confusing. Is that right?

Assuming I'm right so far, then I agree with changing the test in
XLogNeedsFlush() to !XLogInsertAllowed(), as the patch does.

The comment in the patch is describing what's happening, but lost the
"why". Perhaps something like:

"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. The end-of-recovery checkpoint still must
flush WAL like normal, though, so check !XLogInsertAllowed(). This
check must be consistent with XLogFlush()."

The commit message is vague about whether there's a live bug or not; I
suggest clarifying that the inconsistency between the two functions
could have been a bug but wasn't. Also, I generally use past tense for
the stuff that's being fixed and present tense for what the patch does.

One loose end: there was some discussion about the times when
LocalMinRecoveryPoint is valid/correct. I'm not sure I entirely
understood -- is that no longer a concern?

Regards,
    Jeff Davis