Re: Incorrect logic in XLogNeedsFlush()

Chao Li <li.evan.chao@gmail.com>

From: Chao Li <li.evan.chao@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Melanie Plageman <melanieplageman@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Jeff Davis <pgsql@j-davis.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Andres Freund <andres@anarazel.de>
Date: 2025-09-18T00:11:08Z
Lists: pgsql-hackers

> On Sep 18, 2025, at 07:20, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Tue, Sep 16, 2025 at 09:40:50AM +0900, Michael Paquier wrote:
>> As a whole, the patch looks like a good balance, able to satisfy the
>> new case you want to handle, Melanie.  I am guessing that you'd want
>> to tweak it and apply it yourself, so please feel free.
> 
> Hearing nothing, I'd like to move ahead with this improvement.  I have
> tweaked a bit the comments, as suggested.  If one switches the check
> of XLogNeedsFlush() from XLogInsertAllowed() to RecoveryInProgress(),
> the recovery test 015 blows up as expected.
> 
> Any opinions or more word-smithing required?
> --
> Michael
> <v3-0001-Make-XLogFlush-and-XLogNeedsFlush-decision-more-c.patch>


My 5 cents:

```
+	 * XLogInsertAllowed() is used as an end-of-recovery checkpoint is
+	 * launched while recovery is still in progress, RecoveryInProgress()
```

“XLogInsertAllowed() is used as an end-of-recovery checkpoint is launched” sounds like XLogInsertAllowed() is being “used as a checkpoint,” how about rephrase as:

XLogInsertAllowed() is needed because an end-of-recovery checkpoint can be launched while recovery is still in progress,

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/




Commits

  1. Reorder XLogNeedsFlush() checks to be more consistent

  2. Make XLogFlush() and XLogNeedsFlush() decision-making more consistent