From 43da6d5013224ec263490c3d21f471d02204494e Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Fri, 25 Aug 2023 17:09:38 +0200 Subject: [PATCH v2 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/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 e4756f8be21..1b160d140aa 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -1254,7 +1254,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: 36e4419d1f1ef06bba58a28a870aaaa8de73bb46 -- 2.34.1