Re: Incorrect logic in XLogNeedsFlush()

On Wed, Sep 17, 2025 at 7:20 PM 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?

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