From f25b6d27480c90894547d55a41c57664e37f461c Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Fri, 25 Aug 2023 17:09:38 +0200 Subject: [PATCH v6 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 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. It doesn't need to be an error as long as we send the correct RowDescription. Then clients can use that to handle the case when the result types or column counts change in between EXECUTE calls. Most clients already do this anyway, and for any that don't it should not be a big issue to implement it, since it requires no extra round-trip and it's already needed for normal queries anyway. This patch allows 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 | 23 ++++++++++++++++++++++- src/backend/tcop/pquery.c | 25 ++++++++++++++++++++++++- src/backend/utils/cache/plancache.c | 5 ----- src/test/regress/expected/plancache.out | 24 ++++++++++++++++++------ src/test/regress/sql/plancache.sql | 7 +++---- 5 files changed, 67 insertions(+), 17 deletions(-) diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 76f48b13d20..471b980a953 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -1254,7 +1254,28 @@ exec_simple_query(const char *query_string) format = 1; /* BINARY */ } } - PortalSetResultFormat(portal, 1, &format); + + if (portal->commandTag == CMDTAG_EXECUTE) + { + /* + * For EXECUTE 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. + */ + Assert(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 0c45fcf318f..419e0a7584c 100644 --- a/src/backend/tcop/pquery.c +++ b/src/backend/tcop/pquery.c @@ -569,7 +569,14 @@ PortalStart(Portal portal, ParamListInfo params, PlannedStmt *pstmt = PortalGetPrimaryStmt(portal); Assert(pstmt->commandType == CMD_UTILITY); - portal->tupDesc = UtilityTupleDescriptor(pstmt->utilityStmt); + + /* + * For EXECUTE queries tupDesc will be filled by + * FillPortalStore later because it might change due to + * replanning when ExecuteQuery calls GetCachedPlan. + */ + if (portal->commandTag != CMDTAG_EXECUTE) + portal->tupDesc = UtilityTupleDescriptor(pstmt->utilityStmt); } /* @@ -1030,6 +1037,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 delay this for EXECUTE queries, because we only know the + * tuple once we run GetCachedPlan. Since 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 5af1a168ec2..c652ad49d4b 100644 --- a/src/backend/utils/cache/plancache.c +++ b/src/backend/utils/cache/plancache.c @@ -740,11 +740,6 @@ RevalidateCachedQuery(CachedPlanSource *plansource, else if (resultDesc == NULL || plansource->resultDesc == NULL || !equalRowTypes(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: 2ec005b4e29740f0d36e6646d149af192328b2ff -- 2.34.1