At Tue, 12 Nov 2019 11:17:26 +0900, Michael Paquier <mich...@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 <mich...@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