Feedback-ID: ia2694551:Fastmail
Date: Mon, 30 Jan 2023 14:06:09 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: dynamic result sets support in extended query protocol
Message-ID: <20230130130609.mjswqhzqidc3cojl@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="luu7aruazautdsq4"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e7a8f472-9664-d33f-848f-5bf1fa0586b1@enterprisedb.com>
Archived-At: <https://www.postgresql.org/message-id/20230130130609.mjswqhzqidc3cojl%40alvherre.pgsql>
Precedence: bulk


--luu7aruazautdsq4
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2022-Nov-22, Peter Eisentraut wrote:

> I added tests using the new psql \bind command to test this functionality in
> the extended query protocol, which showed that this got broken since I first
> wrote this patch.  This "blame" is on the pipeline mode in libpq patch
> (acb7e4eb6b1c614c68a62fb3a6a5bba1af0a2659).  I need to spend more time on
> this and figure out how to repair it.

Applying this patch, your test queries seem to work correctly.

This is quite WIP, especially because there's a couple of scenarios
uncovered by tests that I'd like to ensure correctness about, but if you
would like to continue adding tests for extended query and dynamic
result sets, it may be helpful.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"How strange it is to find the words "Perl" and "saner" in such close
proximity, with no apparent sense of irony. I doubt that Larry himself
could have managed it."         (ncm, http://lwn.net/Articles/174769/)

--luu7aruazautdsq4
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="libpq-fix.patch.txt"

diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index ec62550e38..b530c51ccd 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -2109,19 +2109,19 @@ PQgetResult(PGconn *conn)
 			break;
 
 		case PGASYNC_READY:
+			res = pqPrepareAsyncResult(conn);
 
 			/*
-			 * For any query type other than simple query protocol, we advance
-			 * the command queue here.  This is because for simple query
-			 * protocol we can get the READY state multiple times before the
-			 * command is actually complete, since the command string can
-			 * contain many queries.  In simple query protocol, the queue
-			 * advance is done by fe-protocol3 when it receives ReadyForQuery.
+			 * When an error has occurred, we consume one command from the
+			 * queue for each result we return.  (Normally, the command would
+			 * be consumed as each result is read from the server.)
 			 */
 			if (conn->cmd_queue_head &&
-				conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE)
+				(conn->error_result ||
+				 (conn->result != NULL &&
+				  conn->result->resultStatus == PGRES_FATAL_ERROR)))
 				pqCommandQueueAdvance(conn);
-			res = pqPrepareAsyncResult(conn);
+
 			if (conn->pipelineStatus != PQ_PIPELINE_OFF)
 			{
 				/*
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 8ab6a88416..2ed74aa0f1 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -205,6 +205,10 @@ pqParseInput3(PGconn *conn)
 							pqSaveErrorResult(conn);
 						}
 					}
+					if (conn->cmd_queue_head &&
+						conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE)
+						pqCommandQueueAdvance(conn);
+
 					if (conn->result)
 						strlcpy(conn->result->cmdStatus, conn->workBuffer.data,
 								CMDSTATUS_LEN);
@@ -231,6 +235,7 @@ pqParseInput3(PGconn *conn)
 						else
 						{
 							conn->pipelineStatus = PQ_PIPELINE_ON;
+							pqCommandQueueAdvance(conn);
 							conn->asyncStatus = PGASYNC_READY;
 						}
 					}
@@ -257,6 +262,7 @@ pqParseInput3(PGconn *conn)
 							pqSaveErrorResult(conn);
 						}
 					}
+					/* XXX should we advance the command queue here? */
 					conn->asyncStatus = PGASYNC_READY;
 					break;
 				case '1':		/* Parse Complete */
@@ -274,6 +280,7 @@ pqParseInput3(PGconn *conn)
 								pqSaveErrorResult(conn);
 							}
 						}
+						pqCommandQueueAdvance(conn);
 						conn->asyncStatus = PGASYNC_READY;
 					}
 					break;
@@ -324,6 +331,10 @@ pqParseInput3(PGconn *conn)
 						 * really possible with the current backend.) We stop
 						 * parsing until the application accepts the current
 						 * result.
+						 *
+						 * Note that we must have already read one 'T' message
+						 * previously for the same command, so we do not
+						 * advance the command queue here.
 						 */
 						conn->asyncStatus = PGASYNC_READY;
 						return;
@@ -355,6 +366,7 @@ pqParseInput3(PGconn *conn)
 							}
 						}
 						conn->asyncStatus = PGASYNC_READY;
+						pqCommandQueueAdvance(conn);
 					}
 					break;
 				case 't':		/* Parameter Description */
@@ -593,14 +605,20 @@ getRowDescriptions(PGconn *conn, int msgLength)
 	/*
 	 * If we're doing a Describe, we're done, and ready to pass the result
 	 * back to the client.
+	 *
+	 * XXX this coding here is a bit suspicious ...
 	 */
-	if ((!conn->cmd_queue_head) ||
-		(conn->cmd_queue_head &&
-		 conn->cmd_queue_head->queryclass == PGQUERY_DESCRIBE))
+	if (!conn->cmd_queue_head)
 	{
 		conn->asyncStatus = PGASYNC_READY;
 		return 0;
 	}
+	else if (conn->cmd_queue_head->queryclass == PGQUERY_DESCRIBE)
+	{
+		conn->asyncStatus = PGASYNC_READY;
+		pqCommandQueueAdvance(conn);
+		return 0;
+	}
 
 	/*
 	 * We could perform additional setup for the new result set here, but for

--luu7aruazautdsq4--