On Tue, Nov 12, 2019 at 7:47 AM Michael Paquier <mich...@paquier.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 <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 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