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

From: Andres Freund <andres(at)anarazel(dot)de>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Cc: Maksim Milyutin <milyutinma(at)gmail(dot)com>, "tsunakawa(dot)takay(at)fujitsu(dot)com" <tsunakawa(dot)takay(at)fujitsu(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Zhihong Yu <zyu(at)yugabyte(dot)com>
Subject: Re: Add client connection check during the execution of the query
Date: 2021-12-11 05:11:01
Message-ID: 20211211051101.mui5qtx3r5ixwkfb@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2021-12-11 05:32:24 Re: parallel vacuum comments
Previous Message Amit Kapila 2021-12-11 04:55:36 Re: Non-superuser subscription owners