Hi Hayato Kuroda,
Thanks for the patch. I think there are some non-fdw extensions out there which could benefit from this logic. That's why I want to first learn some more about high-level design/goals of the patch a little more.
+/*
+ * Call callbacks for checking remote servers.
+ */
+void
+CallCheckingRemoteServersCallbacks(void)
Why do we run this periodically, but not when a specific connection is going to be used? Wouldn't running it periodically prevent detecting some already-closed sockets at the time of the connection used (e.g., we checked 10 seconds ago, the server died 5 seconds ago)?
In other words, what is the trade-off for calling pgfdw_connection_check_internal() inside GetConnection() when we are about to use a "cached" connection? I think that might simplify the patch as well.
+/*
+ * Helper function for pgfdw_connection_check
+ */
+static bool
+pgfdw_connection_check_internal(PGconn *conn)
+{
Can we have this function/logic on Postgres core, so that other extensions can also use?
+ AddWaitEventToSet(eventset, WL_SOCKET_CLOSED, PQsocket(conn), NULL, NULL);
What if PQsocket(conn) returns -1? Maybe we move certain connection state checks into pgfdw_connection_check_internal() such that it is more generic? I can think of checks like: conn!=NULL, PQsocket(conn) != PGINVALID_SOCKET, PQstatus == CONNECTION_OK
+ eventset = CreateWaitEventSet(CurrentMemoryContext, 1);
+ AddWaitEventToSet(eventset, WL_SOCKET_CLOSED, PQsocket(conn), NULL, NULL);
+
+ WaitEventSetWait(eventset, 0, &events, 1, 0);
+
+ if (events.events & WL_SOCKET_CLOSED)
+ {
+ FreeWaitEventSet(eventset);
+ return false;
+ }
+ FreeWaitEventSet(eventset);
Do you see any performance implications of creating/freeing waitEventSets per check? I wonder if we can somehow re-use the same waitEventSet by modifyWaitEvent? I guess no, but still, if this check causes a performance implication, can we somehow cache 1 waitEventSet per connection?
Thanks,
Onder KALACI
Developing the Citus extension @Microsoft
