On 12.06.2021 07:24, Thomas Munro wrote:

On Fri, Apr 30, 2021 at 2:23 PM Thomas Munro <thomas.mu...@gmail.com> wrote:
Here's something I wanted to park here to look into for the next
cycle:  it turns out that kqueue's EV_EOF flag also has the right
semantics for this.  That leads to the idea of exposing the event via
the WaitEventSet API, and would the bring
client_connection_check_interval feature to 6/10 of our OSes, up from
2/10.  Maybe Windows' FD_CLOSE event could get us up to 7/10, not
sure.
Rebased.  Added documentation tweak and a check to reject the GUC on
unsupported OSes.


Good work. I have tested your patch on Linux and FreeBSD on three basic cases: client killing, cable breakdown (via manipulations with firewall) and silent closing client connection before completion of previously started query in asynchronous manner. And all works fine.

Some comments from me:

 bool
 pq_check_connection(void)
 {
-#if defined(POLLRDHUP)
+    WaitEvent events[3];


3 is looks like as magic constant. We might to specify a constant for all event groups in FeBeWaitSet.


+    ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetLatchPos, WL_LATCH_SET, NULL);
+    ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos, WL_SOCKET_CLOSED, NULL);
+    rc = WaitEventSetWait(FeBeWaitSet, 0, events, lengthof(events), 0);
+    ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetLatchPos, WL_LATCH_SET, MyLatch);

AFAICS, side effect to (FeBeWaitSet->events[FeBeWaitSetSocketPos]).events by setting WL_SOCKET_CLOSED value under calling of pq_check_connection() function doesn't have negative impact later, does it? That is, all WaitEventSetWait() calls have to setup socket events on its own from scratch.


--
Regards,
Maksim Milyutin



Reply via email to