From 3bfc47cfa333158c2e0a2e0603e311d141fc473b 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 v3 1/3] 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/postgres.c             | 26 ++++++++++++++++++++++++-
 src/backend/tcop/pquery.c               | 16 +++++++++++++++
 src/backend/utils/cache/plancache.c     |  5 -----
 src/test/regress/expected/plancache.out | 24 +++++++++++++++++------
 src/test/regress/sql/plancache.sql      |  7 +++----
 5 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 21b9763183e..8ea654a4c26 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1252,7 +1252,31 @@ exec_simple_query(const char *query_string)
 					format = 1; /* BINARY */
 			}
 		}
-		PortalSetResultFormat(portal, 1, &format);
+
+		if (portal->strategy == PORTAL_UTIL_SELECT)
+		{
+			/*
+			 * For SELECT-like queries we clear the tupDesc now, and it will
+			 * be 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. This also means that we cannot set
+			 * the result format for the output tuple yet, so we temporarily
+			 * store the desired format in portal->formats. Then after
+			 * creating the actual tupDesc we call PortalSetResultFormat,
+			 * using this format.
+			 */
+			FreeTupleDesc(portal->tupDesc);
+			portal->tupDesc = NULL;
+
+			portal->formats = (int16 *) MemoryContextAlloc(portal->portalContext,
+														   sizeof(int16));
+			portal->formats[0] = format;
+		}
+		else
+		{
+			PortalSetResultFormat(portal, 1, &format);
+		}
 
 		/*
 		 * Now we can create the destination receiver object.
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 5565f200c3d..4f7633faf46 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -1030,6 +1030,22 @@ FillPortalStore(Portal portal, bool isTopLevel)
 		case PORTAL_UTIL_SELECT:
 			PortalRunUtility(portal, linitial_node(PlannedStmt, portal->stmts),
 							 isTopLevel, true, treceiver, &qc);
+
+			if (!portal->tupDesc)
+			{
+				/*
+				 * 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.
+				 */
+				PlannedStmt *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: ac22a9545ca906e70a819b54e76de38817c93aaf
-- 
2.34.1

