Content-Type: multipart/mixed; boundary="------------PMkcelwlfkdmhuTeuWAjrH8q"
Message-ID: <d40a5cd0-2722-40c5-8755-12e9e811fa3c@iki.fi>
Date: Mon, 11 Mar 2024 16:44:32 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
Subject: Disconnect if socket cannot be put into non-blocking mode
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Archived-At: <https://www.postgresql.org/message-id/d40a5cd0-2722-40c5-8755-12e9e811fa3c%40iki.fi>
Precedence: bulk

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

While self-reviewing my "Refactoring backend fork+exec code" patches, I 
noticed this in pq_init():

> 	/*
> 	 * In backends (as soon as forked) we operate the underlying socket in
> 	 * nonblocking mode and use latches to implement blocking semantics if
> 	 * needed. That allows us to provide safely interruptible reads and
> 	 * writes.
> 	 *
> 	 * Use COMMERROR on failure, because ERROR would try to send the error to
> 	 * the client, which might require changing the mode again, leading to
> 	 * infinite recursion.
> 	 */
> #ifndef WIN32
> 	if (!pg_set_noblock(MyProcPort->sock))
> 		ereport(COMMERROR,
> 				(errmsg("could not set socket to nonblocking mode: %m")));
> #endif
> 
> #ifndef WIN32
> 
> 	/* Don't give the socket to any subprograms we execute. */
> 	if (fcntl(MyProcPort->sock, F_SETFD, FD_CLOEXEC) < 0)
> 		elog(FATAL, "fcntl(F_SETFD) failed on socket: %m");
> #endif

Using COMMERROR here seems bogus. Firstly, if there was a problem with 
recursion, surely the elog(FATAL) that follows would also be wrong. But 
more seriously, it's not cool to continue using the connection as if 
everything is OK, if we fail to put it into non-blocking mode. We should 
disconnect. (COMMERROR merely logs the error, it does not bail out like 
ERROR does)

Barring objections, I'll commit and backpatch the attached to fix that.

-- 
Heikki Linnakangas
Neon (https://neon.tech)
--------------PMkcelwlfkdmhuTeuWAjrH8q
Content-Type: text/x-patch; charset=UTF-8;
 name="0001-Disconnect-if-socket-cannot-be-put-into-non-blocking.patch"
Content-Disposition: attachment;
 filename*0="0001-Disconnect-if-socket-cannot-be-put-into-non-blocking.pa";
 filename*1="tch"
Content-Transfer-Encoding: base64

RnJvbSBmYzczNzU4Njc4M2EwNTY3MmZmOGViOTZlMjQ1ZGZkZWVhZGNkNTA2IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBIZWlra2kgTGlubmFrYW5nYXMgPGhlaWtraS5saW5u
YWthbmdhc0Bpa2kuZmk+CkRhdGU6IE1vbiwgMTEgTWFyIDIwMjQgMTY6MzY6MzcgKzAyMDAK
U3ViamVjdDogW1BBVENIIDEvMV0gRGlzY29ubmVjdCBpZiBzb2NrZXQgY2Fubm90IGJlIHB1
dCBpbnRvIG5vbi1ibG9ja2luZyBtb2RlCgpDb21taXQgMzg3ZGExODg3NCBtb3ZlZCB0aGUg
Y29kZSB0byBwdXQgc29ja2V0IGludG8gbm9uLWJsb2NraW5nIG1vZGUKZnJvbSBzb2NrZXRf
c2V0X25vbmJsb2NraW5nKCkgaW50byB0aGUgb25lLXRpbWUgaW5pdGlhbGl6YXRpb24KZnVu
Y3Rpb24sIHBnX2luaXQoKS4gSW4gc29ja2V0X3NldF9ub25ibG9ja2luZygpLCB0aGVyZSBp
bmRlZWQgd2FzIGEKcmlzayBvZiByZWN1cnNpb24gb24gZmFpbHVyZSBsaWtlIHRoZSBjb21t
ZW50IHNhaWQsIGJ1dCBpbiBwZ19pbml0KCksCkVSUk9SIG9yIEZBVEFMIGlzIGZpbmUuIFRo
ZXJlJ3MgZXZlbiBhbm90aGVyIGVsb2coRkFUQUwpIGp1c3QgYWZ0ZXIKdGhpcywgaWYgc2V0
dGluZyBGRF9DTE9FWEVDIGZhaWxzLgoKTm90ZSB0aGF0IENPTU1FUlJPUiBtZXJlbHkgbG9n
Z2VkIHRoZSBlcnJvciwgaXQgZGlkIG5vdCBjbG9zZSB0aGUKY29ubmVjdGlvbiwgc28gaWYg
cHV0dGluZyB0aGUgc29ja2V0IHRvIG5vbi1ibG9ja2luZyBtb2RlIGZhaWxlZCB3ZQp3b3Vs
ZCB1c2UgdGhlIGNvbm5lY3Rpb24gYW55d2F5LiBZb3UgbWlnaHQgbm90IGltbWVkaWF0ZWx5
IG5vdGljZSwKYmVjYXVzZSBtb3N0IHNvY2tldCBvcGVyYXRpb25zIGluIGEgcmVndWxhciBi
YWNrZW5kIHdhaXQgZm9yIHRoZQpzb2NrZXQgdG8gYmVjb21lIHJlYWRhYmxlL3dyaXRhYmxl
IGFueXdheS4gQnV0IGUuZy4gcmVwbGljYXRpb24gd2lsbApiZSBxdWl0ZSBicm9rZW4uCgpC
YWNrcGF0Y2ggdG8gYWxsIHN1cHBvcnRlZCB2ZXJzaW9ucy4KCkRpc2N1c3Npb246IHh4Ci0t
LQogc3JjL2JhY2tlbmQvbGlicHEvcHFjb21tLmMgfCA2ICstLS0tLQogMSBmaWxlIGNoYW5n
ZWQsIDEgaW5zZXJ0aW9uKCspLCA1IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy9i
YWNrZW5kL2xpYnBxL3BxY29tbS5jIGIvc3JjL2JhY2tlbmQvbGlicHEvcHFjb21tLmMKaW5k
ZXggYzYwNmJmMzQ0NzMuLmQ5ZTQ5Y2E3MDI4IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9s
aWJwcS9wcWNvbW0uYworKysgYi9zcmMvYmFja2VuZC9saWJwcS9wcWNvbW0uYwpAQCAtMTg5
LDE0ICsxODksMTAgQEAgcHFfaW5pdCh2b2lkKQogCSAqIG5vbmJsb2NraW5nIG1vZGUgYW5k
IHVzZSBsYXRjaGVzIHRvIGltcGxlbWVudCBibG9ja2luZyBzZW1hbnRpY3MgaWYKIAkgKiBu
ZWVkZWQuIFRoYXQgYWxsb3dzIHVzIHRvIHByb3ZpZGUgc2FmZWx5IGludGVycnVwdGlibGUg
cmVhZHMgYW5kCiAJICogd3JpdGVzLgotCSAqCi0JICogVXNlIENPTU1FUlJPUiBvbiBmYWls
dXJlLCBiZWNhdXNlIEVSUk9SIHdvdWxkIHRyeSB0byBzZW5kIHRoZSBlcnJvciB0bwotCSAq
IHRoZSBjbGllbnQsIHdoaWNoIG1pZ2h0IHJlcXVpcmUgY2hhbmdpbmcgdGhlIG1vZGUgYWdh
aW4sIGxlYWRpbmcgdG8KLQkgKiBpbmZpbml0ZSByZWN1cnNpb24uCiAJICovCiAjaWZuZGVm
IFdJTjMyCiAJaWYgKCFwZ19zZXRfbm9ibG9jayhNeVByb2NQb3J0LT5zb2NrKSkKLQkJZXJl
cG9ydChDT01NRVJST1IsCisJCWVyZXBvcnQoRkFUQUwsCiAJCQkJKGVycm1zZygiY291bGQg
bm90IHNldCBzb2NrZXQgdG8gbm9uYmxvY2tpbmcgbW9kZTogJW0iKSkpOwogI2VuZGlmCiAK
LS0gCjIuMzkuMgoK

--------------PMkcelwlfkdmhuTeuWAjrH8q--