On Wed, Sep 24, 2025 at 7:58 AM Dilip Kumar <[email protected]> wrote:
>
> On Fri, Sep 19, 2025 at 10:58 AM Michael Paquier <[email protected]> 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
From c263f1d1e044d9d3bc03fa4928c94d9951788ace Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 24 Sep 2025 16:57:40 -0400
Subject: [PATCH] Reorder XLogNeedsFlush() checks to be more consistent
In recovery XLogNeedsFlush() checks the minimum recovery point instead
of the flush pointer. The same condition checks are used when updating
the minimum recovery point but written in reverse order. Make the order
of checks consistent between XLogNeedsFlush() and
UpdateMinRecoveryPoint() -- which is invoked during recovery by
XLogFlush().
This improves clarity and does not change functionality.
Discussion: https://postgr.es/m/aMIHNRTP6Wj6vw1s%40paquier.xyz
---
src/backend/access/transam/xlog.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index eac1de75ed0..3929b2ff224 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3134,6 +3134,10 @@ XLogNeedsFlush(XLogRecPtr record)
*/
if (!XLogInsertAllowed())
{
+ /* Quick exit if already known to be updated or cannot be updated */
+ if (!updateMinRecoveryPoint || record <= LocalMinRecoveryPoint)
+ return false;
+
/*
* An invalid minRecoveryPoint means that we need to recover all the
* WAL, i.e., we're doing crash recovery. We never modify the control
@@ -3143,11 +3147,10 @@ XLogNeedsFlush(XLogRecPtr record)
* it has not replayed all WAL available when doing crash recovery.
*/
if (XLogRecPtrIsInvalid(LocalMinRecoveryPoint) && InRecovery)
+ {
updateMinRecoveryPoint = false;
-
- /* Quick exit if already known to be updated or cannot be updated */
- if (record <= LocalMinRecoveryPoint || !updateMinRecoveryPoint)
return false;
+ }
/*
* Update local copy of minRecoveryPoint. But if the lock is busy,
--
2.43.0
