Subject: WIP: libpq: add a possibility to not send D(escribe) when executing a prepared statement

From: Ivan Trofimov <[email-masked]>
Date: Wed, 22 Nov 2023 01:58:48 +0300

Hi!

Currently libpq sends B(ind), D(escribe), E(execute), S(ync) when 
executing a prepared statement.

The response for that D message is a RowDescription, which doesn't 
change during prepared

statement lifetime (with the attributes format being an exception, as 
they aren't know before execution) .

In a presumably very common case of repeatedly executing the same 
statement, this leads to

both client and server parsing/sending exactly the same RowDescritpion 
data over and over again.

Instead, library user could acquire a statement result RowDescription 
once (via PQdescribePrepared),

and reuse it in subsequent calls to PQexecPrepared and/or its async 
friends. Doing it this way saves

a measurable amount of CPU for both client and server and saves a lot of 
network traffic, for example:

when selecting a single row from a table with 30 columns, where each 
column has 10-symbols name, and

every value in a row is a 10-symbols TEXT, i'm seeing an amount of bytes 
sent to client decreased

by a factor of 2.8, and the CPU time client spends in userland decreased 
by a factor of ~1.5.

The patch attached adds a family of functions

PQsendQueryPreparedPredescribed, PQgetResultPredescribed, 
PQisBusyPredescribed,

which allow a user to do just that.

If the idea seems reasonable i'd be happy to extend these to 
PQexecPrepared as well and cover it with tests.

P.S. This is my first time ever sending a patch via email, so please 
don't hesitate to point at mistakes

i'm doing in the process.
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 850734ac96..989354ca06 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -191,3 +191,6 @@ PQclosePrepared           188
 PQclosePortal             189
 PQsendClosePrepared       190
 PQsendClosePortal         191
+PQsendQueryPreparedPredescribed     192
+PQgetResultPredescribed             193
+PQisBusyPredescribed                194
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 04610ccf5e..a18bead9e6 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -72,8 +72,19 @@ static int	PQsendQueryGuts(PGconn *conn,
 							const char *const *paramValues,
 							const int *paramLengths,
 							const int *paramFormats,
-							int resultFormat);
-static void parseInput(PGconn *conn);
+							int resultFormat,
+							bool sendDescribe);
+static int	PQsendQueryPreparedInternal(PGconn *conn,
+										const char *stmtName,
+										int nParams,
+										const char *const *paramValues,
+										const int *paramLengths,
+										const int *paramFormats,
+										int resultFormat,
+										bool sendDescribe);
+static int	PQisBusyInternal(PGconn *conn, PGresult *description);
+static PGresult *PQgetResultInternal(PGconn *conn, PGresult *description);
+static void parseInput(PGconn *conn, PGresult *description);
 static PGresult *getCopyResult(PGconn *conn, ExecStatusType copytype);
 static bool PQexecStart(PGconn *conn);
 static PGresult *PQexecFinish(PGconn *conn);
@@ -1528,7 +1539,8 @@ PQsendQueryParams(PGconn *conn,
 						   paramValues,
 						   paramLengths,
 						   paramFormats,
-						   resultFormat);
+						   resultFormat,
+						   true /* send Describe */ );
 }
 
 /*
@@ -1643,6 +1655,26 @@ PQsendQueryPrepared(PGconn *conn,
 					const int *paramLengths,
 					const int *paramFormats,
 					int resultFormat)
+{
+	return PQsendQueryPreparedInternal(conn,
+									   stmtName,
+									   nParams,
+									   paramValues,
+									   paramLengths,
+									   paramFormats,
+									   resultFormat,
+									   true /* send Describe */ );
+}
+
+int
+PQsendQueryPreparedInternal(PGconn *conn,
+							const char *stmtName,
+							int nParams,
+							const char *const *paramValues,
+							const int *paramLengths,
+							const int *paramFormats,
+							int resultFormat,
+							bool sendDescribe)
 {
 	if (!PQsendQueryStart(conn, true))
 		return 0;
@@ -1668,7 +1700,50 @@ PQsendQueryPrepared(PGconn *conn,
 						   paramValues,
 						   paramLengths,
 						   paramFormats,
-						   resultFormat);
+						   resultFormat,
+						   sendDescribe);
+}
+
+int
+PQsendQueryPreparedPredescribed(PGconn *conn,
+								const char *stmtName,
+								int nParams,
+								const char *const *paramValues,
+								const int *paramLengths,
+								const int *paramFormats,
+								int resultFormat,
+								PGresult *description)
+{
+	int			i;
+	int			nFields;
+	int			send_result;
+
+	if (!description)
+	{
+		libpq_append_conn_error(conn, "query result description is a null pointer");
+		return 0;
+	}
+
+	send_result = PQsendQueryPreparedInternal(conn,
+											  stmtName,
+											  nParams,
+											  paramValues,
+											  paramLengths,
+											  paramFormats,
+											  resultFormat,
+											  false /* send Describe */ );
+
+	/* We only need to adjust attributes format if send succeeded */
+	if (send_result && description->attDescs != NULL)
+	{
+		nFields = description->numAttributes;
+		for (i = 0; i < nFields; i++)
+		{
+			description->attDescs[i].format = resultFormat;
+		}
+	}
+
+	return send_result;
 }
 
 /*
@@ -1766,7 +1841,8 @@ PQsendQueryGuts(PGconn *conn,
 				const char *const *paramValues,
 				const int *paramLengths,
 				const int *paramFormats,
-				int resultFormat)
+				int resultFormat,
+				bool sendDescribe)
 {
 	int			i;
 	PGcmdQueueEntry *entry;
@@ -1873,12 +1949,15 @@ PQsendQueryGuts(PGconn *conn,
 	if (pqPutMsgEnd(conn) < 0)
 		goto sendFailed;
 
-	/* construct the Describe Portal message */
-	if (pqPutMsgStart(PqMsg_Describe, conn) < 0 ||
-		pqPutc('P', conn) < 0 ||
-		pqPuts("", conn) < 0 ||
-		pqPutMsgEnd(conn) < 0)
-		goto sendFailed;
+	if (sendDescribe)
+	{
+		/* construct the Describe Portal message */
+		if (pqPutMsgStart(PqMsg_Describe, conn) < 0 ||
+			pqPutc('P', conn) < 0 ||
+			pqPuts("", conn) < 0 ||
+			pqPutMsgEnd(conn) < 0)
+			goto sendFailed;
+	}
 
 	/* construct the Execute message */
 	if (pqPutMsgStart(PqMsg_Execute, conn) < 0 ||
@@ -1990,9 +2069,9 @@ PQconsumeInput(PGconn *conn)
  * Note that this function will NOT attempt to read more data from the backend.
  */
 static void
-parseInput(PGconn *conn)
+parseInput(PGconn *conn, PGresult *description)
 {
-	pqParseInput3(conn);
+	pqParseInput3Predescribed(conn, description);
 }
 
 /*
@@ -2002,12 +2081,18 @@ parseInput(PGconn *conn)
 
 int
 PQisBusy(PGconn *conn)
+{
+	return PQisBusyInternal(conn, NULL);
+}
+
+static int
+PQisBusyInternal(PGconn *conn, PGresult *description)
 {
 	if (!conn)
 		return false;
 
 	/* Parse any available data, if our state permits. */
-	parseInput(conn);
+	parseInput(conn, description);
 
 	/*
 	 * PQgetResult will return immediately in all states except BUSY.  Also,
@@ -2020,6 +2105,12 @@ PQisBusy(PGconn *conn)
 	return conn->asyncStatus == PGASYNC_BUSY && conn->status != CONNECTION_BAD;
 }
 
+int
+PQisBusyPredescribed(PGconn *conn, PGresult *description)
+{
+	return PQisBusyInternal(conn, description);
+}
+
 /*
  * PQgetResult
  *	  Get the next PGresult produced by a query.  Returns NULL if no
@@ -2033,6 +2124,12 @@ PQisBusy(PGconn *conn)
  */
 PGresult *
 PQgetResult(PGconn *conn)
+{
+	return PQgetResultInternal(conn, NULL);
+}
+
+static PGresult *
+PQgetResultInternal(PGconn *conn, PGresult *description)
 {
 	PGresult   *res;
 
@@ -2040,7 +2137,7 @@ PQgetResult(PGconn *conn)
 		return NULL;
 
 	/* Parse any available data, if our state permits. */
-	parseInput(conn);
+	parseInput(conn, description);
 
 	/* If not ready to return something, block until we are. */
 	while (conn->asyncStatus == PGASYNC_BUSY)
@@ -2078,7 +2175,7 @@ PQgetResult(PGconn *conn)
 		}
 
 		/* Parse it. */
-		parseInput(conn);
+		parseInput(conn, description);
 
 		/*
 		 * If we had a write error, but nothing above obtained a query result
@@ -2182,6 +2279,12 @@ PQgetResult(PGconn *conn)
 	return res;
 }
 
+PGresult *
+PQgetResultPredescribed(PGconn *conn, PGresult *description)
+{
+	return PQgetResultInternal(conn, description);
+}
+
 /*
  * getCopyResult
  *	  Helper for PQgetResult: generate result for COPY-in-progress cases
@@ -2638,7 +2741,7 @@ PQnotifies(PGconn *conn)
 		return NULL;
 
 	/* Parse any available data to see if we can extract NOTIFY messages. */
-	parseInput(conn);
+	parseInput(conn, NULL);
 
 	event = conn->notifyHead;
 	if (event)
@@ -2677,7 +2780,7 @@ PQputCopyData(PGconn *conn, const char *buffer, int nbytes)
 	 * input data into the input buffer happens down inside pqSendSome, but
 	 * it's not authorized to get rid of the data again.)
 	 */
-	parseInput(conn);
+	parseInput(conn, NULL);
 
 	if (nbytes > 0)
 	{
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 5613c56b14..8fa5853f84 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -55,6 +55,7 @@ static void reportErrorPosition(PQExpBuffer msg, const char *query,
 								int loc, int encoding);
 static int	build_startup_packet(const PGconn *conn, char *packet,
 								 const PQEnvironmentOption *options);
+static void pqParseInput3Internal(PGconn *conn, PGresult *description);

 /*
@@ -62,8 +63,8 @@ static int	build_startup_packet(const PGconn *conn, char *packet,
  * until input is exhausted or a stopping state is reached.
  * Note that this function will NOT attempt to read more data from the backend.
  */
-void
-pqParseInput3(PGconn *conn)
+static void
+pqParseInput3Internal(PGconn *conn, PGresult *description)
 {
 	char		id;
 	int			msgLength;
@@ -402,11 +403,38 @@ pqParseInput3(PGconn *conn)
 					}
 					else
 					{
-						/* Set up to report error at end of query */
-						libpq_append_conn_error(conn, "server sent data (\"D\" message) without prior row description (\"T\" message)");
-						pqSaveErrorResult(conn);
-						/* Discard the unexpected message */
-						conn->inCursor += msgLength;
+						if (!description)
+						{
+							/* Set up to report error at end of query */
+							libpq_append_conn_error(conn,
+													"server sent data (\"D\" message) without prior row description (\"T\" message)");
+							pqSaveErrorResult(conn);
+							/* Discard the unexpected message */
+							conn->inCursor += msgLength;
+						}
+						else
+						{
+							/*
+							 * We didn't receive row description from the
+							 * server, but we have externally provided
+							 * description, use that.
+							 */
+							if (conn->result == NULL)
+							{
+								conn->result = PQmakeEmptyPGresult(conn, PGRES_TUPLES_OK);
+							}
+							if (!conn->result)
+							{
+								libpq_append_conn_error(conn, "out of memory");
+								pqSaveErrorResult(conn);
+							}
+							else
+							{
+								PQsetResultAttrs(conn->result, description->numAttributes, description->attDescs);
+								if (getAnotherTuple(conn, msgLength))
+									return;
+							}
+						}
 					}
 					break;
 				case PqMsg_CopyInResponse:
@@ -478,6 +506,18 @@ pqParseInput3(PGconn *conn)
 	}
 }
 
+void
+pqParseInput3(PGconn *conn)
+{
+	pqParseInput3Internal(conn, NULL);
+}
+
+void
+pqParseInput3Predescribed(PGconn *conn, PGresult *description)
+{
+	pqParseInput3Internal(conn, description);
+}
+
 /*
  * handleSyncLoss: clean up after loss of message-boundary sync
  *
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 97762d56f5..573b68fea2 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -462,11 +462,21 @@ extern int	PQsendQueryPrepared(PGconn *conn,
 								const int *paramLengths,
 								const int *paramFormats,
 								int resultFormat);
+extern int	PQsendQueryPreparedPredescribed(PGconn *conn,
+											const char *stmtName,
+											int nParams,
+											const char *const *paramValues,
+											const int *paramLengths,
+											const int *paramFormats,
+											int resultFormat,
+											PGresult *description);
 extern int	PQsetSingleRowMode(PGconn *conn);
 extern PGresult *PQgetResult(PGconn *conn);
+extern PGresult *PQgetResultPredescribed(PGconn *conn, PGresult *description);
 
 /* Routines for managing an asynchronous query */
 extern int	PQisBusy(PGconn *conn);
+extern int	PQisBusyPredescribed(PGconn *conn, PGresult *description);
 extern int	PQconsumeInput(PGconn *conn);
 
 /* Routines for pipeline mode management */
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index c745facfec..df91c39fbe 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -706,6 +706,7 @@ extern int	PQsendQueryContinue(PGconn *conn, const char *query);
 extern char *pqBuildStartupPacket3(PGconn *conn, int *packetlen,
 								   const PQEnvironmentOption *options);
 extern void pqParseInput3(PGconn *conn);
+extern void pqParseInput3Predescribed(PGconn *conn, PGresult *description);
 extern int	pqGetErrorNotice3(PGconn *conn, bool isError);
 extern void pqBuildErrorMessage3(PQExpBuffer msg, const PGresult *res,
 								 PGVerbosity verbosity, PGContextVisibility show_context);

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Wed, 22 Nov 2023 11:46:27 +0500

Hi Ivan,

thank you for the patch.

> On 22 Nov 2023, at 03:58, Ivan Trofimov <[email-masked]> wrote:
> 
> Currently libpq sends B(ind), D(escribe), E(execute), S(ync) when executing a prepared statement.
> The response for that D message is a RowDescription, which doesn't change during prepared
> statement lifetime (with the attributes format being an exception, as they aren't know before execution) .
From my POV the idea seems reasonable (though I’m not a real libpq expert).
BTW some drivers also send Describe even before Bind. This creates some fuss for routing connection poolers.

> In a presumably very common case of repeatedly executing the same statement, this leads to
> both client and server parsing/sending exactly the same RowDescritpion data over and over again.
> Instead, library user could acquire a statement result RowDescription once (via PQdescribePrepared),
> and reuse it in subsequent calls to PQexecPrepared and/or its async friends.
But what if query result structure changes? Will we detect this error gracefully and return correct error?

Best regards, Andrey Borodin.

---

From: Ivan Trofimov <[email-masked]>
Date: Sun, 26 Nov 2023 19:38:49 +0300

> But what if query result structure changes? Will we detect this error gracefully and return correct error?

Afaik changing prepared statement result structure is prohibited by
Postgres server-side, and should always lead to "ERROR: cached plan
must not change result type", see src/test/regress/sql/plancache.sql.

So yes, from the libpq point of view this is just an server error, which
would be given to the user, the patch shouldn't change any behavior here.

The claim about this always being a server-side error better be
reassured from someone from the Postgres team, of course.

---

From: Tom Lane <[email-masked]>
Date: Sun, 26 Nov 2023 12:13:45 -0500

> must not change result type", see src/test/regress/sql/plancache.sql.

Independently of whether we're willing to guarantee that that will
never change, I think this patch is basically a bad idea as presented.
It adds a whole new set of programmer-error possibilities, and I doubt
it saves enough in typical cases to justify creating such a foot-gun.

Moreover, it will force us to devote attention to the problem of
keeping libpq itself from misbehaving badly in the inevitable
situation that somebody passes the wrong tuple descriptor.
That is developer effort we could better spend elsewhere.

I say this as somebody who deliberately designed the v3 protocol
to allow clients to skip Describe steps if they were confident
they knew the query result type.  I am not disavowing that choice;
I just think that successful use of that option requires a client-
side coding structure that allows tying a previously-obtained
tuple descriptor to the current query with confidence.  The proposed
API fails badly at that, or at least leaves it up to the end-user
programmer while providing no tools to help her get it right.

Instead, I'm tempted to suggest having PQprepare/PQexecPrepared
maintain a cache that maps statement name to result tupdesc, so that
this is all handled internally to libpq.  The main hole in that idea
is that it's possible to issue PREPARE, DEALLOCATE, etc via PQexec, so
that a user could possibly redefine a prepared statement without libpq
noticing it.  Maybe that's not a big issue.  For a little more safety,
we could add some extra step that the library user has to take to
enable caching of result tupdescs, whereupon it's definitely caller
error if she does that and then changes the statement behind our back.

BTW, the submitted patch lacks both documentation and tests.
For a feature like this, there is much to be said for writing
the documentation *first*.  Having to explain how to use something
often helps you figure out weak spots in your initial design.

			regards, tom lane