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 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. -- Michael
signature.asc
Description: PGP signature