Re: Incorrect logic in XLogNeedsFlush()

On Mon, Sep 15, 2025 at 10:58 AM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Sun, Sep 14, 2025 at 4:10 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > Updated as per suggestion
>
> I like the idea of adding an assert to keep the two in sync. However, two things
>
> 1) Since XLogNeedsFlush() refreshes LogwrtResult, is it possible for
> this assert to fail because the flush pointer was updated? I realize
> that that is the case with my original patch as well. If this is true,
> perhaps there is not a way to use XLogNeedsFlush() after XLogFlush()
> -- only before as a check to see if we must call XLogFlush().

Oops, ignore this -- obviously the flush pointer can only move
forwards :) So, this is not an issue.

> 2) You've added the assert at the bottom which will only cover the
> flush pointer case and not the min recovery point case. Maybe that's
> good enough though...

I actually think this is fine also. The assert will ensure that
XLogNeedsFlush() and XLogFlush() stay in sync WRT which path is taken
(either comparing to the min recovery point or comparing to the flush
pointer). It doesn't need to recheck what XLogFlush() just checked.

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.

- Melanie