Re: Incorrect logic in XLogNeedsFlush()

On Fri, Sep 12, 2025 at 9:47 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Sep 12, 2025 at 08:45:36AM +0530, Dilip Kumar wrote:
> > On Fri, Sep 12, 2025 at 8:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >> +1, it really makes XLogFlush() to directly check using
> >> XLogNeedsFlush() after adding the "WAL inserts are allowed" check in
> >> XLogNeedsFlush(), this is the best way to avoid any inconsistencies in
> >> future as well.
> >
> > I tried with the attached patch, at least check-world reports no issue.
>
> @@ -2797,7 +2797,7 @@ XLogFlush(XLogRecPtr record)
>         }
>
>      /* Quick exit if already known flushed */
> -    if (record <= LogwrtResult.Flush)
> +    if (!XLogNeedsFlush(record))
>          return;
>
> Hmm, no.  You are making more expensive a check that is written to be
> cheap.  I was more thinking about an assertion at the bottom of
> XLogFlush() once a flush is completed.  Input and ideas from others
> are of course welcome on the matter.

Yeah, asserting it at the end makes sense, as we can ensure that
XLogFlush() and XLogNeedsFlush() agree on the same thing without
adding additional overhead.  Here is the revised one.

--
Regards,
Dilip Kumar
Google