Re: Incorrect logic in XLogNeedsFlush()

On Wed, Sep 24, 2025 at 7:58 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Sep 19, 2025 at 10:58 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > Do we want to make the order of the checks to be more consistent in
> > both routines?  These would require a separate set of double-checks
> > and review, but while we're looking at this area of the code we may as
> > tweak it more..
>
> I see both routines have the same order i.e. first check if
> (!XLogInsertAllowed()) and then if (record <= LogwrtResult.Flush),
> what am I missing?

I think he's referring to the order of checks inside
UpdateMinRecoveryPoint(). Something like the attached patch.

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.

Is this roughly what you were thinking, Michael?

diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 3929b2ff224..73d4e62e4eb 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2714,7 +2714,7 @@ UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force)
     * available is replayed in this case.  This also saves from extra locks
     * taken on the control file from the startup process.
     */
-   if (XLogRecPtrIsInvalid(LocalMinRecoveryPoint) && InRecovery)
+   if (GetRecoveryState() == RECOVERY_STATE_CRASH)
    {
        updateMinRecoveryPoint = false;
        return;
@@ -3146,7 +3146,7 @@ XLogNeedsFlush(XLogRecPtr record)
         * which cannot update its local copy of minRecoveryPoint as long as
         * it has not replayed all WAL available when doing crash recovery.
         */
-       if (XLogRecPtrIsInvalid(LocalMinRecoveryPoint) && InRecovery)
+       if (GetRecoveryState() == RECOVERY_STATE_CRASH)
        {
            updateMinRecoveryPoint = false;
            return false;

Because we reordered the checks, we will set updateMinRecoveryPoint to
false the first time and avoid the spin lock acquisition in
GetRecoveryState() happening more than once.

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

- Melanie

[1] https://www.postgresql.org/message-id/aMIHNRTP6Wj6vw1s%40paquier.xyz