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:
ERROR: cached plan must not change result type
This requirement was not documented anywhere and it
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.
Some relevant previous discussions:
[1]:
https://www.postgresql.org/message-id/flat/CAB%3DJe-GQOW7kU9Hn3AqP1vhaZg_wE9Lz6F4jSp-7cm9_M6DyVA%40mail.gmail.com
[2]:
https://stackoverflow.com/questions/2783813/postgres-error-cached-plan-must-not-change-result-type
[3]:
https://stackoverflow.com/questions/42119365/how-to-avoid-cached-plan-must-not-change-result-type-error
[4]: https://github.com/pgjdbc/pgjdbc/pull/451
[5]: https://github.com/pgbouncer/pgbouncer/pull/845#discussion_r1305295551
[6]: https://github.com/jackc/pgx/issues/927
[7]:
https://elixirforum.com/t/postgrex-errors-with-cached-plan-must-not-change-result-type-during-migration/51235/2
[8]: https://github.com/rails/rails/issues/12330
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
From 261d3a8244a52765a3af635278e9c7a3b0081d93 Mon Sep 17 00:00:00 2001
From: Jelte Fennema <jelte.fennema@microsoft.com>
Date: Fri, 25 Aug 2023 19:48:24 +0200
Subject: [PATCH v1 2/2] Completely remove fixed_result from CachedPlanSource
Because of the previous patch, no fixed_result plans exist anymore. This
removes all references. It's done in a separate commit to ease
reviewing, if this gets merged we might want to squash them together.
---
src/backend/commands/prepare.c | 12 +-----------
src/backend/executor/spi.c | 6 ++----
src/backend/tcop/postgres.c | 6 +-----
src/backend/utils/cache/plancache.c | 7 +------
src/include/utils/plancache.h | 4 +---
5 files changed, 6 insertions(+), 29 deletions(-)
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 18f70319fc4..da3115beb99 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -127,8 +127,7 @@ PrepareQuery(ParseState *pstate, PrepareStmt *stmt,
nargs,
NULL,
NULL,
- CURSOR_OPT_PARALLEL_OK, /* allow parallel mode */
- true); /* fixed result */
+ CURSOR_OPT_PARALLEL_OK); /* allow parallel mode */
/*
* Save the results.
@@ -165,10 +164,6 @@ ExecuteQuery(ParseState *pstate,
/* Look it up in the hash table */
entry = FetchPreparedStatement(stmt->name, true);
- /* Shouldn't find a non-fixed-result cached plan */
- if (!entry->plansource->fixed_result)
- elog(ERROR, "EXECUTE does not support variable-result cached plans");
-
/* Evaluate parameters, if any */
if (entry->plansource->num_params > 0)
{
@@ -469,7 +464,6 @@ FetchPreparedStatementResultDesc(PreparedStatement *stmt)
* Since we don't allow prepared statements' result tupdescs to change,
* there's no need to worry about revalidating the cached plan here.
*/
- Assert(stmt->plansource->fixed_result);
if (stmt->plansource->resultDesc)
return CreateTupleDescCopy(stmt->plansource->resultDesc);
else
@@ -591,10 +585,6 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/* Look it up in the hash table */
entry = FetchPreparedStatement(execstmt->name, true);
- /* Shouldn't find a non-fixed-result cached plan */
- if (!entry->plansource->fixed_result)
- elog(ERROR, "EXPLAIN EXECUTE does not support variable-result cached plans");
-
query_string = entry->plansource->query_string;
/* Evaluate parameters, if any */
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 33975687b38..52128290741 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2285,8 +2285,7 @@ _SPI_prepare_plan(const char *src, SPIPlanPtr plan)
plan->nargs,
plan->parserSetup,
plan->parserSetupArg,
- plan->cursor_options,
- false); /* not fixed result */
+ plan->cursor_options); /* not fixed result */
plancache_list = lappend(plancache_list, plansource);
}
@@ -2522,8 +2521,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
plan->nargs,
plan->parserSetup,
plan->parserSetupArg,
- plan->cursor_options,
- false); /* not fixed result */
+ plan->cursor_options);
}
/*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index e4756f8be21..183170d90be 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1554,8 +1554,7 @@ exec_parse_message(const char *query_string, /* string to execute */
numParams,
NULL,
NULL,
- CURSOR_OPT_PARALLEL_OK, /* allow parallel mode */
- true); /* fixed result */
+ CURSOR_OPT_PARALLEL_OK); /* allow parallel mode */
/* If we got a cancel signal during analysis, quit */
CHECK_FOR_INTERRUPTS();
@@ -2629,9 +2628,6 @@ exec_describe_statement_message(const char *stmt_name)
errmsg("unnamed prepared statement does not exist")));
}
- /* Prepared statements shouldn't have changeable result descs */
- Assert(psrc->fixed_result);
-
/*
* If we are in aborted transaction state, we can't run
* SendRowDescriptionMessage(), because that needs catalog accesses.
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index a6fbe235381..1106215b781 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -203,7 +203,6 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
plansource->parserSetup = NULL;
plansource->parserSetupArg = NULL;
plansource->cursor_options = 0;
- plansource->fixed_result = false;
plansource->resultDesc = NULL;
plansource->context = source_context;
plansource->query_list = NIL;
@@ -271,7 +270,6 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->parserSetup = NULL;
plansource->parserSetupArg = NULL;
plansource->cursor_options = 0;
- plansource->fixed_result = false;
plansource->resultDesc = NULL;
plansource->context = CurrentMemoryContext;
plansource->query_list = NIL;
@@ -346,8 +344,7 @@ CompleteCachedPlan(CachedPlanSource *plansource,
int num_params,
ParserSetupHook parserSetup,
void *parserSetupArg,
- int cursor_options,
- bool fixed_result)
+ int cursor_options)
{
MemoryContext source_context = plansource->context;
MemoryContext oldcxt = CurrentMemoryContext;
@@ -430,7 +427,6 @@ CompleteCachedPlan(CachedPlanSource *plansource,
plansource->parserSetup = parserSetup;
plansource->parserSetupArg = parserSetupArg;
plansource->cursor_options = cursor_options;
- plansource->fixed_result = fixed_result;
plansource->resultDesc = PlanCacheComputeResultDesc(querytree_list);
MemoryContextSwitchTo(oldcxt);
@@ -1547,7 +1543,6 @@ CopyCachedPlan(CachedPlanSource *plansource)
newsource->parserSetup = plansource->parserSetup;
newsource->parserSetupArg = plansource->parserSetupArg;
newsource->cursor_options = plansource->cursor_options;
- newsource->fixed_result = plansource->fixed_result;
if (plansource->resultDesc)
newsource->resultDesc = CreateTupleDescCopy(plansource->resultDesc);
else
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 916e59d9fef..36fd0104aec 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -104,7 +104,6 @@ typedef struct CachedPlanSource
ParserSetupHook parserSetup; /* alternative parameter spec method */
void *parserSetupArg;
int cursor_options; /* cursor options used for planning */
- bool fixed_result; /* disallow change in result tupdesc? */
TupleDesc resultDesc; /* result type; NULL = doesn't return tuples */
MemoryContext context; /* memory context holding all above */
/* These fields describe the current analyzed-and-rewritten query tree: */
@@ -201,8 +200,7 @@ extern void CompleteCachedPlan(CachedPlanSource *plansource,
int num_params,
ParserSetupHook parserSetup,
void *parserSetupArg,
- int cursor_options,
- bool fixed_result);
+ int cursor_options);
extern void SaveCachedPlan(CachedPlanSource *plansource);
extern void DropCachedPlan(CachedPlanSource *plansource);
--
2.34.1
