Subject: Re: BUG #16678: The ecpg connect/test5 test sometimes fails on
 Windows
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
References: <16678-253e48d34dc0c376@postgresql.org>
 <434311.1603135136@sss.pgh.pa.us>
 <f558fb39-ce9e-a516-d7c3-fc94fb581711@gmail.com>
 <383222.1603561145@sss.pgh.pa.us>
From: Alexander Lakhin <exclusion@gmail.com>
Message-ID: <c550c1b0-45e2-9f71-1472-f3f22bbc702a@gmail.com>
Date: Sun, 25 Oct 2020 00:00:00 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <383222.1603561145@sss.pgh.pa.us>
Content-Type: multipart/mixed;
 boundary="------------C835F28B0FCDAF56A0F9DF6F"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------C835F28B0FCDAF56A0F9DF6F
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

24.10.2020 20:39, Tom Lane wrote:
> Alexander Lakhin <exclusion@gmail.com> writes:
>> So I've also tested on Windows the following version:
>>         secure_close(MyProcPort);
>>         shutdown(MyProcPort->sock, SD_SEND);
>>         for(;;) {
>>             char buffer[1000];
>>             int res = recv(MyProcPort->sock, buffer, 1000, 0);
>>             if (res <= 0)
>>                 break;
>>         }
>>         closesocket(MyProcPort->sock);
>> And it works too.
> I'm afraid this cure is probably worse than the disease, because
> now the backend's exiting is held hostage by whether the client
> closes its socket (and the resulting FIN reaches us, which it
> might not if there's a network problem).
On a next level of this game we could make something like that:
        secure_close(MyProcPort);
        timeout = 1000; // 1 sec
        setsockopt(MyProcPort->sock, SOL_SOCKET, SO_RCVTIMEO,
(char*)&timeout, sizeof(timeout));
        shutdown(MyProcPort->sock, SD_SEND);
        for(;;) {
            char buffer[1000];
            int res = recv(MyProcPort->sock, buffer, 1000, 0);
            if (res <= 0) {
                break;
            }
        }
        closesocket(MyProcPort->sock);
(I can't get this timeout working yet.)

But I'm inclined to stay on the previous level with "shutdown &&
closesocket" as recommended for server side:
https://docs.microsoft.com/en-us/windows/win32/winsock/graceful-shutdown-linger-options-and-socket-closure-2
Please look at the draft patch.

By the way, the delay added to pqReadData():
    pg_usleep(10000L); // Wait for 10 ms.
makes the src/test/recovery test fail on Linux too (pg_ctl can't stop
the primary node).
So it seems that slow reading from a socket breaks things not only on
Windows and the fix should be more comprehensive.

Best regards,
Alexander

--------------C835F28B0FCDAF56A0F9DF6F
Content-Type: text/x-patch; charset=UTF-8;
 name="graceful_socket_shutdown.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="graceful_socket_shutdown.patch"

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index ac986c0505..df681df103 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -285,16 +285,26 @@ socket_close(int code, Datum arg)
 		 * call this, so this is safe when interrupting BackendInitialize().
 		 */
 		secure_close(MyProcPort);
+#ifdef WIN32
+		/*
+		 * Shutdown and close the socket explicitly to allow clients to receive
+		 * a message sent just before the backend exit, e.g., a FATAL error.
+		 * Otherwise Windows resets the connection and doesn't transmit data
+		 * when the process dies.
+		 */
+		shutdown(MyProcPort->sock, SD_BOTH);
+		closesocket(MyProcPort->sock);
+#endif
 
 		/*
-		 * Formerly we did an explicit close() here, but it seems better to
-		 * leave the socket open until the process dies.  This allows clients
-		 * to perform a "synchronous close" if they care --- wait till the
-		 * transport layer reports connection closure, and you can be sure the
-		 * backend has exited.
+		 * Formerly we did an explicit close() here on other OSes too, but it
+		 * seems better to leave the socket open until the process dies.
+		 * This allows clients to perform a "synchronous close" if they care
+		 *  --- wait till the transport layer reports connection closure,
+		 * and you can be sure the backend has exited.
 		 *
-		 * We do set sock to PGINVALID_SOCKET to prevent any further I/O,
-		 * though.
+		 * We do set sock to PGINVALID_SOCKET to prevent any further I/O
+		 * in any case.
 		 */
 		MyProcPort->sock = PGINVALID_SOCKET;
 	}

--------------C835F28B0FCDAF56A0F9DF6F--