From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Daniel Gustafsson <daniel@yesql.se>,
 Thomas Munro <thomas.munro@gmail.com>,
        byavuz81@gmail.com,
        PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>,
        Heikki Linnakangas <heikki.linnakangas@iki.fi>,
        Michael Paquier <michael@paquier.xyz>
Subject: Re: BUG #17391: While using --with-ssl=openssl and
 PG_TEST_EXTRA='ssl' options, SSL tests fail on OpenBSD 7.0
In-reply-to: <2077573.1644599439@sss.pgh.pa.us>
References: <C44CE9FB-B37E-47D3-924B-4AF808AE98B1@yesql.se>
 <887CBA28-0942-4084-BE09-FBE0D6A1DB53@yesql.se>
 <621771.1644098845@sss.pgh.pa.us> <635760.1644108785@sss.pgh.pa.us>
 <1113966.1644280235@sss.pgh.pa.us>
 <C112C400-2CB5-4DCA-A699-00DC0333B665@yesql.se>
 <1537890.1644462214@sss.pgh.pa.us> <1952254.1644540297@sss.pgh.pa.us>
 <20220211011025.ek7exh6owpzjyudn@alap3.anarazel.de>
 <1955929.1644542728@sss.pgh.pa.us>
 <20220211015455.mkl6cvstttp5zfco@alap3.anarazel.de>
 <1959196.1644544971@sss.pgh.pa.us> <2077573.1644599439@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Fri, 11 Feb 2022 12:10:39 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2187142.1644616371.0@sss.pgh.pa.us>
Date: Fri, 11 Feb 2022 16:54:54 -0500
Message-ID: <2187263.1644616494@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2187263.1644616494%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2187142.1644616371.1@sss.pgh.pa.us>

I wrote:
>> Hmm, maybe.  It sure *looks* like we need to do

>> -	return conn->asyncStatus == PGASYNC_BUSY || conn->write_failed;
>> +	return conn->asyncStatus == PGASYNC_BUSY && !conn->write_failed;

> Dunno anything about the Windows angle, but after hacking together
> a reproducer for this, I'm fairly convinced that this is indeed a bug.

After further study, I now think that checking write_failed here is
the wrong thing entirely, because it's irrelevant to what we want to
do, which is keep reading until we've collected a server message or
seen read EOF.

What we do need to do though is check to see if we've closed the
socket.  That's because places like libpqwalreceiver.c assume
that if PQisBusy is true, then PQsocket() must be valid:

    while (PQisBusy(streamConn))
    {
        int            rc;

        rc = WaitLatchOrSocket(MyLatch,
                               WL_EXIT_ON_PM_DEATH | WL_SOCKET_READABLE |
                               WL_LATCH_SET,
                               PQsocket(streamConn),
                               0,
                               WAIT_EVENT_LIBPQWALRECEIVER_RECEIVE);

It seems like the existing coding in PQisBusy could allow that to fail.
I scraped the buildfarm for instances of the "cannot wait on socket
event without a socket" error that latch.c would emit if that happened,
but found none going back three months.  That's perhaps because of the
other behavior I noted that if the walsender crashes, we'll probably
report write failure immediately in PQsendQuery.  So maybe this is
mostly hypothetical in practice, but I think what we actually want
here is as attached.

In any case, I still don't see a way for this error to result in
an infinite loop, so it doesn't seem like an explanation for the
Windows problems.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="PQisBusy-fix.patch"; charset="us-ascii"
Content-ID: <2187142.1644616371.2@sss.pgh.pa.us>
Content-Description: PQisBusy-fix.patch

diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 59121873d2..9afd4d88b4 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1957,10 +1957,14 @@ PQisBusy(PGconn *conn)
 	parseInput(conn);
 
 	/*
-	 * PQgetResult will return immediately in all states except BUSY, or if we
-	 * had a write failure.
+	 * PQgetResult will return immediately in all states except BUSY.  Also,
+	 * if we've detected read EOF and dropped the connection, we can expect
+	 * that PQgetResult will fail immediately.  Note that we do *not* check
+	 * conn->write_failed here --- once that's become set, we know we have
+	 * trouble, but we need to keep trying to read until we have a complete
+	 * server message or detect read EOF.
 	 */
-	return conn->asyncStatus == PGASYNC_BUSY || conn->write_failed;
+	return conn->asyncStatus == PGASYNC_BUSY && conn->status != CONNECTION_BAD;
 }
 
 /*

------- =_aaaaaaaaaa0--