On Thu, Sep 30, 2021 at 3:56 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > I am not able to understand some parts of that patch. > > + If the sleep is shorter > + * than KEEPALIVE_TIMEOUT milliseconds, we skip sending a keepalive to > + * prevent it from getting too-frequent. > + */ > + if (MyWalSnd->flush < sentPtr && > + MyWalSnd->write < sentPtr && > + !waiting_for_ping_response) > + { > + if (sleeptime > KEEPALIVE_TIMEOUT) > + { > + int r; > + > + r = WalSndWait(wakeEvents, KEEPALIVE_TIMEOUT, > + WAIT_EVENT_WAL_SENDER_WAIT_WAL); > + > + if (r != 0) > + continue; > + > + sleeptime -= KEEPALIVE_TIMEOUT; > + } > + > + WalSndKeepalive(false); > > It claims to skip sending keepalive if the sleep time is shorter than > KEEPALIVE_TIMEOUT (a new threshold) but the above code seems to > suggest it sends in both cases. What am I missing? >
The comment does seem to be wrong. The way I see it, if the calculated sleeptime is greater than KEEPALIVE_TIMEOUT, then it first sleeps for up to KEEPALIVE_TIMEOUT milliseconds and skips sending a keepalive if something happens (i.e. the socket becomes readable/writeable) during that time (WalSendWait will return non-zero in that case). Otherwise, it sends a keepalive and sleeps for (sleeptime - KEEPALIVE_TIMEOUT), then loops around again ... > Also, more to the point this special keep_alive seems to be sent for > synchronous replication and walsender shutdown as they can expect > updated locations. You haven't given any reason (theory) why those two > won't be impacted due to this change? I am aware that for synchronous > replication, we wake waiters while ProcessStandbyReplyMessage but I am > not sure how it helps with wal sender shutdown? I think we need to > know the reasons for this message and then need to see if the change > has any impact on the same. > Yes, I'm not sure about the possible impacts, still looking at it. Regards, Greg Nancarrow Fujitsu Australia