Horton

Thread

Commits

  1. Disconnect if socket cannot be put into non-blocking mode

  1. Disconnect if socket cannot be put into non-blocking mode

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-03-11T14:44:32Z

    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)

  2. Re: Disconnect if socket cannot be put into non-blocking mode

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-03-12T08:49:31Z

    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)

  3. Re: Disconnect if socket cannot be put into non-blocking mode

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-12T11:09:52Z

    > On 12 Mar 2024, at 09:49, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

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

Sorry for being slow to review, but +1 on this change.

--
Daniel Gustafsson