From b1a58082f2b226b37d237580e33b52438d480f48 Mon Sep 17 00:00:00 2001
From: Jelte Fennema <jelte.fennema@microsoft.com>
Date: Fri, 25 Aug 2023 17:09:38 +0200
Subject: [PATCH v1 1/2] Support prepared statement invalidation when result
 types change

The cached plan for a prepared statements can get invalidated when DDL
changes the tables used in the query, or when search_path changes. When
this happens the prepared statement can still be executed, but it will
be replanned in the new context. This means that the prepared statement
will do something different e.g. in case of search_path changes it will
select data from a completely different table. This won't throw an
error, because it is considered the responsibility of the operator and
query writers that the query will still do the intended thing.

However, we would throw an error if the the result of the query is of a
different type than it was before. This was not documented anywhere and
can thus be a surprising error to hit. But it's actually not needed for
this to be an error, as long as we send the correct RowDescription there
does not have to be a problem for clients when the result types or
column counts change.

This patch starts to allow a prepared statement to continue to work even
when the result type changes.

Without this change all clients that automatically prepare queries as a
performance optimization will need to handle or avoid the error somehow,
often resulting in deallocating and re-preparing queries when its
usually not necessary. With this change connection poolers can also
safely prepare the same query only once on a connection and share this
one prepared query across clients that prepared that exact same query.
---
 src/backend/tcop/pquery.c               | 46 +++++++++++++++++++------
 src/backend/utils/cache/plancache.c     |  5 ---
 src/test/regress/expected/plancache.out | 24 +++++++++----
 src/test/regress/sql/plancache.sql      |  7 ++--
 4 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 5565f200c3d..ee790009cd2 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -561,23 +561,21 @@ PortalStart(Portal portal, ParamListInfo params,
 
 			case PORTAL_UTIL_SELECT:
 
-				/*
-				 * We don't set snapshot here, because PortalRunUtility will
-				 * take care of it if needed.
-				 */
-				{
-					PlannedStmt *pstmt = PortalGetPrimaryStmt(portal);
-
-					Assert(pstmt->commandType == CMD_UTILITY);
-					portal->tupDesc = UtilityTupleDescriptor(pstmt->utilityStmt);
-				}
-
 				/*
 				 * Reset cursor position data to "start of query"
 				 */
 				portal->atStart = true;
 				portal->atEnd = false;	/* allow fetches */
 				portal->portalPos = 0;
+
+				/*
+				 * The tupDesc is filled in later by FillPortalStore, because
+				 * the tupDesc might change due to replanning when
+				 * ExecuteQuery calls GetCachedPlan. So we should only fetch
+				 * the tupDesc after the query is actually executed.
+				 */
+				portal->tupDesc = NULL;
+
 				break;
 
 			case PORTAL_MULTI_QUERY:
@@ -627,7 +625,20 @@ PortalSetResultFormat(Portal portal, int nFormats, int16 *formats)
 
 	/* Do nothing if portal won't return tuples */
 	if (portal->tupDesc == NULL)
+	{
+		/*
+		 * For SELECT like queries we delay filling in the tupDesc until after
+		 * PortalRunUtility, because we don't know what rows an EXECUTE
+		 * command will return. Because we delay setting tupDesc, we also need
+		 * to delay setting formats. We do this in a pretty hacky way, by
+		 * temporarily setting the portal formats to the passed in formats.
+		 * Then once we fill in tupDesc, we call PortalSetResultFormat again
+		 * with portal->formats to fill in the final formats value.
+		 */
+		if (portal->strategy == PORTAL_UTIL_SELECT)
+			portal->formats = formats;
 		return;
+	}
 	natts = portal->tupDesc->natts;
 	portal->formats = (int16 *)
 		MemoryContextAlloc(portal->portalContext,
@@ -1001,6 +1012,7 @@ FillPortalStore(Portal portal, bool isTopLevel)
 {
 	DestReceiver *treceiver;
 	QueryCompletion qc;
+	PlannedStmt *pstmt;
 
 	InitializeQueryCompletion(&qc);
 	PortalCreateHoldStore(portal);
@@ -1030,6 +1042,18 @@ FillPortalStore(Portal portal, bool isTopLevel)
 		case PORTAL_UTIL_SELECT:
 			PortalRunUtility(portal, linitial_node(PlannedStmt, portal->stmts),
 							 isTopLevel, true, treceiver, &qc);
+
+			/*
+			 * Now fill in the tupDesc and formats fields of the portal, we
+			 * delayed this because for EXECUTE queries we only know the tuple
+			 * after they were executed, because its original plan might get
+			 * replaced with a new one that returns different columns, due to
+			 * the table having changed since the last PREPARE/EXECUTE
+			 * command.
+			 */
+			pstmt = PortalGetPrimaryStmt(portal);
+			portal->tupDesc = UtilityTupleDescriptor(pstmt->utilityStmt);
+			PortalSetResultFormat(portal, 1, portal->formats);
 			break;
 
 		default:
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 7d4168f82f5..a6fbe235381 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -717,11 +717,6 @@ RevalidateCachedQuery(CachedPlanSource *plansource,
 	else if (resultDesc == NULL || plansource->resultDesc == NULL ||
 			 !equalTupleDescs(resultDesc, plansource->resultDesc))
 	{
-		/* can we give a better error message? */
-		if (plansource->fixed_result)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cached plan must not change result type")));
 		oldcxt = MemoryContextSwitchTo(plansource->context);
 		if (resultDesc)
 			resultDesc = CreateTupleDescCopy(resultDesc);
diff --git a/src/test/regress/expected/plancache.out b/src/test/regress/expected/plancache.out
index 4e59188196c..77ae17b074a 100644
--- a/src/test/regress/expected/plancache.out
+++ b/src/test/regress/expected/plancache.out
@@ -49,14 +49,26 @@ EXECUTE prepstmt2(123);
  123 | 4567890123456789
 (2 rows)
 
--- prepared statements should prevent change in output tupdesc,
--- since clients probably aren't expecting that to change on the fly
-ALTER TABLE pcachetest ADD COLUMN q3 bigint;
+-- prepared statements should work even if the output tupdesc changes
+ALTER TABLE pcachetest ADD COLUMN q3 bigint DEFAULT 20;
 EXECUTE prepstmt;
-ERROR:  cached plan must not change result type
+        q1        |        q2         | q3 
+------------------+-------------------+----
+ 4567890123456789 | -4567890123456789 | 20
+ 4567890123456789 |               123 | 20
+              123 |               456 | 20
+              123 |  4567890123456789 | 20
+ 4567890123456789 |  4567890123456789 | 20
+(5 rows)
+
 EXECUTE prepstmt2(123);
-ERROR:  cached plan must not change result type
--- but we're nice guys and will let you undo your mistake
+ q1  |        q2        | q3 
+-----+------------------+----
+ 123 |              456 | 20
+ 123 | 4567890123456789 | 20
+(2 rows)
+
+-- changing it back should also work fine
 ALTER TABLE pcachetest DROP COLUMN q3;
 EXECUTE prepstmt;
         q1        |        q2         
diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql
index 4b2f11dcc64..cab0c42a142 100644
--- a/src/test/regress/sql/plancache.sql
+++ b/src/test/regress/sql/plancache.sql
@@ -27,14 +27,13 @@ CREATE TEMP TABLE pcachetest AS SELECT * FROM int8_tbl ORDER BY 2;
 EXECUTE prepstmt;
 EXECUTE prepstmt2(123);
 
--- prepared statements should prevent change in output tupdesc,
--- since clients probably aren't expecting that to change on the fly
-ALTER TABLE pcachetest ADD COLUMN q3 bigint;
+-- prepared statements should work even if the output tupdesc changes
+ALTER TABLE pcachetest ADD COLUMN q3 bigint DEFAULT 20;
 
 EXECUTE prepstmt;
 EXECUTE prepstmt2(123);
 
--- but we're nice guys and will let you undo your mistake
+-- changing it back should also work fine
 ALTER TABLE pcachetest DROP COLUMN q3;
 
 EXECUTE prepstmt;

base-commit: 1a4fd77db85abac63e178506335aee74625f6499
-- 
2.34.1

