From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: Fabien COELHO <coelho@cri.ensmp.fr>,
        Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: libpq async duplicate error results
In-reply-to: <1262938.1645658107@sss.pgh.pa.us>
References: <202202081500.xl3iwa7doscz@alvherre.pgsql>
 <394797.1645388816@sss.pgh.pa.us> <1262938.1645658107@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Wed, 23 Feb 2022 18:15:07 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1421777.1645723233.0@sss.pgh.pa.us>
Date: Thu, 24 Feb 2022 12:20:38 -0500
Message-ID: <1421785.1645723238@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1421785.1645723238%40sss.pgh.pa.us>
Precedence: bulk

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

Actually ... it now seems to me that there's live bugs in the
interaction between pipeline mode and error-buffer clearing.
Namely, that places like PQsendQueryStart will unconditionally
clear the buffer even though we may be in the middle of a pipeline
sequence, and the buffer might hold an error that's yet to be
reported to the application.  So I think we need something
more like the attached.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-pipeline-errorMessage-reset-2.patch";
	charset="us-ascii"
Content-ID: <1421777.1645723233.2@sss.pgh.pa.us>
Content-Description: fix-pipeline-errorMessage-reset-2.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec=
.c
index 45dddaf556..0c39bc9abf 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1380,10 +1380,7 @@ pqAppendCmdQueueEntry(PGconn *conn, PGcmdQueueEntry=
 *entry)
 			 * state, we don't have to do anything.
 			 */
 			if (conn->asyncStatus =3D=3D PGASYNC_IDLE)
-			{
-				pqClearConnErrorState(conn);
 				pqPipelineProcessQueue(conn);
-			}
 			break;
 	}
 }
@@ -1730,8 +1727,10 @@ PQsendQueryStart(PGconn *conn, bool newQuery)
 =

 	/*
 	 * If this is the beginning of a query cycle, reset the error state.
+	 * However, in pipeline mode with something already queued, the error
+	 * buffer belongs to that command and we shouldn't clear it.
 	 */
-	if (newQuery)
+	if (newQuery && conn->cmd_queue_head =3D=3D NULL)
 		pqClearConnErrorState(conn);
 =

 	/* Don't try to send if we know there's no live connection. */
@@ -2149,11 +2148,8 @@ PQgetResult(PGconn *conn)
 				/*
 				 * We're about to return the NULL that terminates the round of
 				 * results from the current query; prepare to send the results
-				 * of the next query when we're called next.  Also, since this
-				 * is the start of the results of the next query, clear any
-				 * prior error message.
+				 * of the next query when we're called next.
 				 */
-				pqClearConnErrorState(conn);
 				pqPipelineProcessQueue(conn);
 			}
 			break;
@@ -2362,6 +2358,14 @@ PQexecStart(PGconn *conn)
 	if (!conn)
 		return false;
 =

+	/*
+	 * Since this is the beginning of a query cycle, reset the error state.
+	 * However, in pipeline mode with something already queued, the error
+	 * buffer belongs to that command and we shouldn't clear it.
+	 */
+	if (conn->cmd_queue_head =3D=3D NULL)
+		pqClearConnErrorState(conn);
+
 	if (conn->pipelineStatus !=3D PQ_PIPELINE_OFF)
 	{
 		appendPQExpBufferStr(&conn->errorMessage,
@@ -2369,11 +2373,6 @@ PQexecStart(PGconn *conn)
 		return false;
 	}
 =

-	/*
-	 * Since this is the beginning of a query cycle, reset the error state.
-	 */
-	pqClearConnErrorState(conn);
-
 	/*
 	 * Silently discard any prior query result that application didn't eat.
 	 * This is probably poor design, but it's here for backward compatibilit=
y.
@@ -2928,8 +2927,11 @@ PQfn(PGconn *conn,
 =

 	/*
 	 * Since this is the beginning of a query cycle, reset the error state.
+	 * However, in pipeline mode with something already queued, the error
+	 * buffer belongs to that command and we shouldn't clear it.
 	 */
-	pqClearConnErrorState(conn);
+	if (conn->cmd_queue_head =3D=3D NULL)
+		pqClearConnErrorState(conn);
 =

 	if (conn->pipelineStatus !=3D PQ_PIPELINE_OFF)
 	{
@@ -3099,6 +3101,12 @@ pqPipelineProcessQueue(PGconn *conn)
 		conn->cmd_queue_head =3D=3D NULL)
 		return;
 =

+	/*
+	 * Reset the error state.  This and the next couple of steps correspond =
to
+	 * what PQsendQueryStart didn't do for this query.
+	 */
+	pqClearConnErrorState(conn);
+
 	/* Initialize async result-accumulation state */
 	pqClearAsyncResult(conn);
 =

@@ -3809,9 +3817,11 @@ PQsetnonblocking(PGconn *conn, int arg)
 	 * behavior. this is ok because either they are making a transition _fro=
m_
 	 * or _to_ blocking mode, either way we can block them.
 	 *
-	 * Clear error state in case pqFlush adds to it.
+	 * Clear error state in case pqFlush adds to it, unless we're actively
+	 * pipelining, in which case it seems best not to.
 	 */
-	pqClearConnErrorState(conn);
+	if (conn->cmd_queue_head =3D=3D NULL)
+		pqClearConnErrorState(conn);
 =

 	/* if we are going from blocking to non-blocking flush here */
 	if (pqFlush(conn))
@@ -4003,7 +4013,8 @@ PQescapeStringConn(PGconn *conn,
 		return 0;
 	}
 =

-	pqClearConnErrorState(conn);
+	if (conn->cmd_queue_head =3D=3D NULL)
+		pqClearConnErrorState(conn);
 =

 	return PQescapeStringInternal(conn, to, from, length, error,
 								  conn->client_encoding,
@@ -4041,7 +4052,8 @@ PQescapeInternal(PGconn *conn, const char *str, size=
_t len, bool as_ident)
 	if (!conn)
 		return NULL;
 =

-	pqClearConnErrorState(conn);
+	if (conn->cmd_queue_head =3D=3D NULL)
+		pqClearConnErrorState(conn);
 =

 	/* Scan the string for characters that must be escaped. */
 	for (s =3D str; (s - str) < len && *s !=3D '\0'; ++s)
@@ -4306,7 +4318,8 @@ PQescapeByteaConn(PGconn *conn,
 	if (!conn)
 		return NULL;
 =

-	pqClearConnErrorState(conn);
+	if (conn->cmd_queue_head =3D=3D NULL)
+		pqClearConnErrorState(conn);
 =

 	return PQescapeByteaInternal(conn, from, from_length, to_length,
 								 conn->std_strings,

------- =_aaaaaaaaaa0--