At Tue, 12 Nov 2019 11:17:26 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> 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@paquier.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 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.
>
> 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
It seems to me as if it is a fast-path when RecentFlushPtr reached the
target location before enterig the loop. It is frequently called in
(AFAICS) interruptible loops. On that standpoint I vote +1 for Amit.
Or we could shift the stuff of the for loop so that the duplicate code
is placed at the beginning.
> 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.
>
> Personally, my take would be to remove the extra call to
> GetFlushRecPtr() before entering the loop.
>
> > Also, what's up with those useless returns?
>
> Yes, let's rip them out.
It seems to me that the fast-path seems intentional.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
