Re: Incorrect logic in XLogNeedsFlush()

On Mon, Sep 15, 2025 at 03:02:36PM -0400, Melanie Plageman wrote:
> As such, we should clarify the comment above the assert in your patch
> to make it clear that the point of the assert is not to check the
> logic in XLogFlush() but to protect against drift between
> XLogNeedsFlush() and XLogFlush() WRT the comparison logic used.

Agreed.

While looking at this patch, I have planted the following assertion in
XLogNeedsFlush():

@@ -3142,6 +3150,13 @@ XLogNeedsFlush(XLogRecPtr record)
LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
LWLockRelease(ControlFileLock);

+ /*
+ * An invalid minRecoveryPoint can only be accessed in the startup
+ * process or while in single-user mode.
+ */
+ Assert(AmStartupProcess() || !IsUnderPostmaster ||
+ !XLogRecPtrIsInvalid(LocalMinRecoveryPoint));

Then wondered if we should try to push for more efforts about making
XLogNeedsFlush() callable in non-startup non-postmaster processes
while we perform crash recovery (aka mininum recovery point is still
invalid, because XLogFlush() can be called by the checkpointer since
7ff23c6d277d while in crash recovery). However, I cannot get much
excited about this without an actual use case, also knowing that
UpdateMinRecoveryPoint() is coded to be defensive enough to discard
this case.

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.
--
Michael