Message-ID: <5f600c8b-19f2-4cc7-9e9f-65130117223f@iki.fi>
Date: Tue, 12 Mar 2024 10:49:31 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Disconnect if socket cannot be put into non-blocking mode
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <d40a5cd0-2722-40c5-8755-12e9e811fa3c@iki.fi>
In-Reply-To: <d40a5cd0-2722-40c5-8755-12e9e811fa3c@iki.fi>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/5f600c8b-19f2-4cc7-9e9f-65130117223f%40iki.fi>
Precedence: bulk

On 11/03/2024 16:44, Heikki Linnakangas wrote:
> 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.

Committed.

-- 
Heikki Linnakangas
Neon (https://neon.tech)