Re: Incorrect logic in XLogNeedsFlush()

On Wed, Sep 17, 2025 at 7:20 PM Michael Paquier <michael(at)paquier(dot)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?

Commit message looks good.

In terms of comments, I think it is best to update the comment above
XLogNeedsFlush(). Something like :

/*
- * Test whether XLOG data has been flushed up to (at least) the given position.
+ * Test whether XLOG data has been flushed up to (at least) the given position
+ * or whether the minimum recovery point is updated past the given position.
*
- * Returns true if a flush is still needed. (It may be that someone else
- * is already in process of flushing that far, however.)
+ * Returns true if a flush is still needed or if the minimum recovery point
+ * must be updated.
+ *
+ * It is possible that someone else is already in the process of flushing that
+ * far or updating the minimum recovery point that far.

Though maybe that's too literal...

> + /*
> + * Cross-check XLogNeedsFlush(). Some of the checks of XLogFlush() and
> + * XLogNeedsFlush() are duplicated, and this assertion ensures that these
> + * remain consistent.
> + */
> + Assert(!XLogNeedsFlush(record));
> }
>
> /*
> @@ -3114,8 +3121,13 @@ XLogNeedsFlush(XLogRecPtr record)
> * 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.
> + *
> + * XLogInsertAllowed() is used as an end-of-recovery checkpoint is
> + * launched while recovery is still in progress, RecoveryInProgress()
> + * returning true in this case. This check should be consistent with the
> + * one in XLogFlush().
> */

I can't quite parse the wording of the above comment.

- Melanie