Re: Coding in WalSndWaitForWal

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 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