Re: Coding in WalSndWaitForWal

On Tue, Nov 12, 2019 at 7:47 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Nov 11, 2019 at 01:53:40PM -0300, Alvaro Herrera wrote:
> > On 2019-Nov-11, Amit Kapila wrote:
> >
> >> On Mon, Nov 11, 2019 at 7:53 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >>> So your suggestion would be to call GetFlushRecPtr() before the first
> >>> check on RecentFlushPtr and before entering the loop?
> >>
> >> No. What I meant was to keep the current code as-is and have an
> >> additional check on RecentFlushPtr before entering the loop.
>
> Okay, but is that really useful?
>

I think so. It will be useful in cases where the WAL is already
flushed by the WALWriter in the meantime.

> > I noticed that the "return" at the bottom of the function does a
> > SetLatch(), but the other returns do not. Isn't that a bug?
>
> I don't think that it is necessary to set the latch in the first check
> as in this case WalSndWaitForWal() would have gone through its loop to
> set RecentFlushPtr to the last position available already, which would
> have already set the latch. If you add an extra check based on (loc
> <= RecentFlushPtr) as your patch does, then you need to set the
> latch appropriately before returning.
>

This point makes sense to me.

> Anyway, I don't think that there is any reason to do this extra work
> at the beginning of the routine before entering the loop. But there
> is an extra reason not to do that: your patch would prevent more pings
> to be sent, which means less flush LSN updates. If you think that
> the extra check makes sense, then I think that the patch should at
> least clearly document why it is done this way, and why it makes
> sense to do so.
>

I also think adding a comment there would be good.

>
> > Also, what's up with those useless returns?
>
> Yes, let's rip them out.
>

+1.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com