On 08/28/2014 03:47 PM, Kyotaro HORIGUCHI wrote:
To make the code mentioned above (Patch 0002) tidy, rewrite the socket emulation code for win32 backends so that each socket can have its own non-blocking state. (patch 0001)
The first patch that makes non-blocking sockets behave more sanely on Windows seems like a good idea, independently of the second patch. I'm looking at the first patch now, I'll make a separate post about the second patch.
Some concern about this patch, - This patch allows the number of non-blocking socket to be below 64 (FD_SETSIZE) on win32 backend but it seems to be sufficient.
Yeah, that's plenty.
- This patch introduced redundant socket emulation for win32 backend but win32 bare socket for Port is already nonblocking as described so it donsn't seem to be a serious problem on performance. Addition to it, since I don't know the reason why win32/socket.c provides the blocking-mode socket emulation, I decided to preserve win32/socket.c to have blocking socket emulation. Possibly it can be removed.
On Windows, the backend has an emulation layer for POSIX signals, which uses threads and Windows events. The reason win32/socket.c always uses non-blocking mode internally is that it needs to wait for the socket to become readable/writeable, and for the signal-emulation event, at the same time. So no, we can't remove it.
The approach taken in the first patch seems sensible. I changed it to not use FD_SET, though. A custom array seems better, that way we don't need the pgwin32_nonblockset_init() call, we can just use initialize the variable. It's a little bit more code, but it's well-contained in win32/socket.c. Please take a look, to double-check that I didn't screw up.
- Heikki
commit aaaec23b08677baaed900f72db3f9628c0070922 Author: Heikki Linnakangas <heikki.linnakan...@iki.fi> Date: Tue Sep 2 20:05:47 2014 +0300 Improve non-blocking sockets emulation on Windows. diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 605d891..cba79a7 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -795,10 +795,6 @@ pq_set_nonblocking(bool nonblocking) if (MyProcPort->noblock == nonblocking) return; -#ifdef WIN32 - pgwin32_noblock = nonblocking ? 1 : 0; -#else - /* * Use COMMERROR on failure, because ERROR would try to send the error to * the client, which might require changing the mode again, leading to @@ -816,7 +812,6 @@ pq_set_nonblocking(bool nonblocking) ereport(COMMERROR, (errmsg("could not set socket to blocking mode: %m"))); } -#endif MyProcPort->noblock = nonblocking; } diff --git a/src/backend/port/win32/socket.c b/src/backend/port/win32/socket.c index c981169..51982f0 100644 --- a/src/backend/port/win32/socket.c +++ b/src/backend/port/win32/socket.c @@ -3,6 +3,9 @@ * socket.c * Microsoft Windows Win32 Socket Functions * + * Blocking socket functions implemented so they listen on both the socket + * and the signal event, required for signal handling. + * * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group * * IDENTIFICATION @@ -14,18 +17,19 @@ #include "postgres.h" /* - * Indicate if pgwin32_recv() and pgwin32_send() should operate - * in non-blocking mode. - * - * Since the socket emulation layer always sets the actual socket to - * non-blocking mode in order to be able to deliver signals, we must - * specify this in a separate flag if we actually need non-blocking - * operation. - * - * This flag changes the behaviour *globally* for all socket operations, - * so it should only be set for very short periods of time. + * Keep track which sockets are in non-blocking mode. Since the socket + * emulation layer always sets the actual socket to non-blocking mode in + * order to be able to deliver signals, we must track ourselves whether to + * present blocking or non-blocking behavior to the callers of pgwin32_recv() + * and pgwin32_send(). We expect there to be only a few non-blocking sockets, + * so a small array will do. */ -int pgwin32_noblock = 0; +#define MAX_NONBLOCKING_SOCKETS 10 + +static SOCKET nonblocking_sockets[MAX_NONBLOCKING_SOCKETS]; +static int num_nonblocking_sockets = 0; + +static bool pgwin32_socket_is_nonblocking(SOCKET s); #undef socket #undef accept @@ -33,11 +37,57 @@ int pgwin32_noblock = 0; #undef select #undef recv #undef send +#undef closesocket /* - * Blocking socket functions implemented so they listen on both - * the socket and the signal event, required for signal handling. + * Add a socket to the list of non-blocking sockets. */ +void +pgwin32_set_socket_noblock(SOCKET s) +{ + if (pgwin32_socket_is_nonblocking(s)) + return; /* already non-nlocking */ + + if (num_nonblocking_sockets >= MAX_NONBLOCKING_SOCKETS) + elog(ERROR, "too many non-blocking sockets"); + + nonblocking_sockets[num_nonblocking_sockets++] = s; +} + +/* + * Remove a socket from the list of non-blocking sockets. + */ +void +pgwin32_set_socket_block(SOCKET s) +{ + int i; + + for (i = 0; i < num_nonblocking_sockets; i++) + { + if (nonblocking_sockets[i] == s) + { + /* Found. Move the last entry to this slot. */ + if (i != num_nonblocking_sockets - 1) + nonblocking_sockets[i] = + nonblocking_sockets[num_nonblocking_sockets - 1]; + num_nonblocking_sockets--; + break; + } + } +} + +static bool +pgwin32_socket_is_nonblocking(SOCKET s) +{ + int i; + + for (i = 0; i < num_nonblocking_sockets; i++) + { + if (nonblocking_sockets[i] == s) + return true; + } + return false; +} /* * Convert the last socket error code into errno @@ -334,7 +384,7 @@ pgwin32_recv(SOCKET s, char *buf, int len, int f) return -1; } - if (pgwin32_noblock) + if (pgwin32_socket_is_nonblocking(s)) { /* * No data received, and we are in "emulated non-blocking mode", so @@ -420,7 +470,7 @@ pgwin32_send(SOCKET s, const void *buf, int len, int flags) return -1; } - if (pgwin32_noblock) + if (pgwin32_socket_is_nonblocking(s)) { /* * No data sent, and we are in "emulated non-blocking mode", so @@ -645,6 +695,16 @@ pgwin32_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, c /* + * If the socket is in non-blocking mode, remove the entry from the array. + */ +int +pgwin32_closesocket(SOCKET s) +{ + pgwin32_set_socket_block(s); + return closesocket(s); +} + +/* * Return win32 error string, since strerror can't * handle winsock codes */ diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index c7f41a5..32be003 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -3255,22 +3255,9 @@ PgstatCollectorMain(int argc, char *argv[]) /* * Try to receive and process a message. This will not block, * since the socket is set to non-blocking mode. - * - * XXX On Windows, we have to force pgwin32_recv to cooperate, - * despite the previous use of pg_set_noblock() on the socket. - * This is extremely broken and should be fixed someday. */ -#ifdef WIN32 - pgwin32_noblock = 1; -#endif - len = recv(pgStatSock, (char *) &msg, sizeof(PgStat_Msg), 0); - -#ifdef WIN32 - pgwin32_noblock = 0; -#endif - if (len < 0) { if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) diff --git a/src/include/port/win32.h b/src/include/port/win32.h index 550c3ec..e3c0473 100644 --- a/src/include/port/win32.h +++ b/src/include/port/win32.h @@ -368,6 +368,7 @@ void pg_queue_signal(int signum); #define select(n, r, w, e, timeout) pgwin32_select(n, r, w, e, timeout) #define recv(s, buf, len, flags) pgwin32_recv(s, buf, len, flags) #define send(s, buf, len, flags) pgwin32_send(s, buf, len, flags) +#define closesocket(s) pgwin32_closesocket(s) SOCKET pgwin32_socket(int af, int type, int protocol); SOCKET pgwin32_accept(SOCKET s, struct sockaddr * addr, int *addrlen); @@ -375,11 +376,12 @@ int pgwin32_connect(SOCKET s, const struct sockaddr * name, int namelen); int pgwin32_select(int nfds, fd_set *readfs, fd_set *writefds, fd_set *exceptfds, const struct timeval * timeout); int pgwin32_recv(SOCKET s, char *buf, int len, int flags); int pgwin32_send(SOCKET s, const void *buf, int len, int flags); +int pgwin32_closesocket(SOCKET s); const char *pgwin32_socket_strerror(int err); int pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout); - -extern int pgwin32_noblock; +void pgwin32_set_socket_block(SOCKET s); +void pgwin32_set_socket_noblock(SOCKET s); /* in backend/port/win32/security.c */ extern int pgwin32_is_admin(void); diff --git a/src/port/noblock.c b/src/port/noblock.c index 1da0339..c8b35e0 100644 --- a/src/port/noblock.c +++ b/src/port/noblock.c @@ -22,11 +22,21 @@ pg_set_noblock(pgsocket sock) { #if !defined(WIN32) return (fcntl(sock, F_SETFL, O_NONBLOCK) != -1); -#else + +#elif defined(FRONTEND) unsigned long ioctlsocket_ret = 1; /* Returns non-0 on failure, while fcntl() returns -1 on failure */ return (ioctlsocket(sock, FIONBIO, &ioctlsocket_ret) == 0); + +#else + /* + * Sockets in Windows backend processes are wrapped, and blocking mode is + * handled by the emulation layer. See src/backend/port/win32/socket.c for + * details. + */ + pgwin32_set_socket_noblock(sock); + return 1; #endif } @@ -41,10 +51,16 @@ pg_set_block(pgsocket sock) if (flags < 0 || fcntl(sock, F_SETFL, (long) (flags & ~O_NONBLOCK))) return false; return true; -#else + +#elif FRONTEND unsigned long ioctlsocket_ret = 0; /* Returns non-0 on failure, while fcntl() returns -1 on failure */ return (ioctlsocket(sock, FIONBIO, &ioctlsocket_ret) == 0); + +#else + /* See pg_set_noblock */ + pgwin32_set_socket_block(sock); + return 1; #endif }
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers