Re: Add client connection check during the execution of the query

Andres Freund <andres@anarazel.de>

From: Andres Freund <andres@anarazel.de>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Maksim Milyutin <milyutinma@gmail.com>, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, Tatsuo Ishii <ishii@sraoss.co.jp>, Tom Lane <tgl@sss.pgh.pa.us>, Zhihong Yu <zyu@yugabyte.com>
Date: 2021-12-11T05:11:01Z
Lists: pgsql-hackers
Hi,

On 2021-12-11 17:41:34 +1300, Thomas Munro wrote:
> --- a/src/backend/libpq/pqcomm.c
> +++ b/src/backend/libpq/pqcomm.c
> @@ -1959,22 +1959,36 @@ pq_settcpusertimeout(int timeout, Port *port)
>  bool
>  pq_check_connection(void)
>  {
> -#if defined(POLLRDHUP)
> -	/*
> -	 * POLLRDHUP is a Linux extension to poll(2) to detect sockets closed by
> -	 * the other end.  We don't have a portable way to do that without
> -	 * actually trying to read or write data on other systems.  We don't want
> -	 * to read because that would be confused by pipelined queries and COPY
> -	 * data. Perhaps in future we'll try to write a heartbeat message instead.
> -	 */
> -	struct pollfd pollfd;
> +	WaitEvent	event;
>  	int			rc;
>  
> -	pollfd.fd = MyProcPort->sock;
> -	pollfd.events = POLLOUT | POLLIN | POLLRDHUP;
> -	pollfd.revents = 0;
> +	/*
> +	 * Temporarily ignore the latch, so that we can poll for just the one
> +	 * event we care about.
> +	 */
> +	ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetLatchPos, WL_LATCH_SET, NULL);
>  
> -	rc = poll(&pollfd, 1, 0);
> +	PG_TRY();
> +	{
> +		/*
> +		 * It's OK to clobber the socket event to report only the event we're
> +		 * interested in without restoring the previous state afterwards,
> +		 * because every FeBeWaitSet wait site does the same.
> +		 */
> +		ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos, WL_SOCKET_CLOSED,
> +						NULL);
> +		rc = WaitEventSetWait(FeBeWaitSet, 0, &event, 1, 0);
> +	}
> +	PG_FINALLY();
> +	{
> +		/*
> +		 * Restore the latch, so we can't leave FeBeWaitSet in a broken state
> +		 * that ignores latches.
> +		 */
> +		ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetLatchPos, WL_LATCH_SET,
> +						MyLatch);
> +	}
> +	PG_END_TRY();

Yuck. Is there really no better way to deal with this? What kind of errors is
this trying to handle transparently? Afaics this still changes when we'd
e.g. detect postmaster death.

Am I misunderstanding code or comment, or is the comment saying that it's ok
to clobber the wes, but then we actually unclobber it?

Greetings,

Andres Freund



Commits

  1. Add WL_SOCKET_CLOSED for socket shutdown events.

  2. Use WL_SOCKET_CLOSED for client_connection_check_interval.

  3. Detect POLLHUP/POLLRDHUP while running queries.

  4. Improve timeout.c's handling of repeated timeout set/cancel.