From be76d20e65ca4229fa445f9a1054357bcf6c5bb0 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Thu, 7 Sep 2023 18:51:30 +0200 Subject: [PATCH v5 3/3] Support changing argument types of prepared statemnts Prepared statement invalidations might require argument types to change for the prepared statement to continue to be usable. One example of this is when an argument is compared against one of the columns of a table and the type of that column is changed by some DDL or search_path changes. This commit starts storing the argument types that were originally given when preparing the query. That way we can re-resolve the argument types when the prepared statement is revalidated. --- src/backend/commands/prepare.c | 24 +++++--- src/backend/executor/spi.c | 14 +++-- src/backend/tcop/postgres.c | 19 +++++- src/backend/utils/cache/plancache.c | 81 ++++++++++++++++++++----- src/include/utils/plancache.h | 28 ++++++++- src/test/regress/expected/plancache.out | 30 ++++++++- src/test/regress/sql/plancache.sql | 12 +++- 7 files changed, 176 insertions(+), 32 deletions(-) diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c index da3115beb99..f0c9786193e 100644 --- a/src/backend/commands/prepare.c +++ b/src/backend/commands/prepare.c @@ -83,12 +83,6 @@ PrepareQuery(ParseState *pstate, PrepareStmt *stmt, rawstmt->stmt_location = stmt_location; rawstmt->stmt_len = stmt_len; - /* - * Create the CachedPlanSource before we do parse analysis, since it needs - * to see the unmodified raw parse tree. - */ - plansource = CreateCachedPlan(rawstmt, pstate->p_sourcetext, - CreateCommandTag(stmt->query)); /* Transform list of TypeNames to array of type OIDs */ nargs = list_length(stmt->argtypes); @@ -110,6 +104,14 @@ PrepareQuery(ParseState *pstate, PrepareStmt *stmt, } } + /* + * Create the CachedPlanSource before we do parse analysis, since it needs + * to see the unmodified raw parse tree. + */ + plansource = CreateCachedPlan(rawstmt, pstate->p_sourcetext, + argtypes, nargs, + CreateCommandTag(stmt->query)); + /* * Analyze the statement using these parameter types (any parameters * passed in from above us will not be visible to it), allowing @@ -160,10 +162,13 @@ ExecuteQuery(ParseState *pstate, char *query_string; int eflags; long count; + List *revalidationResult; /* Look it up in the hash table */ entry = FetchPreparedStatement(stmt->name, true); + revalidationResult = RevalidateCachedQuery(entry->plansource, NULL); + /* Evaluate parameters, if any */ if (entry->plansource->num_params > 0) { @@ -188,7 +193,7 @@ ExecuteQuery(ParseState *pstate, entry->plansource->query_string); /* Replan if needed, and increment plan refcount for portal */ - cplan = GetCachedPlan(entry->plansource, paramLI, NULL, NULL); + cplan = GetCachedPlan(entry->plansource, paramLI, NULL, NULL, revalidationResult); plan_list = cplan->stmt_list; /* @@ -577,6 +582,7 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es, instr_time planduration; BufferUsage bufusage_start, bufusage; + List *revalidationResult; if (es->buffers) bufusage_start = pgBufferUsage; @@ -585,6 +591,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es, /* Look it up in the hash table */ entry = FetchPreparedStatement(execstmt->name, true); + revalidationResult = RevalidateCachedQuery(entry->plansource, NULL); + query_string = entry->plansource->query_string; /* Evaluate parameters, if any */ @@ -609,7 +617,7 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es, /* Replan if needed, and acquire a transient refcount */ cplan = GetCachedPlan(entry->plansource, paramLI, - CurrentResourceOwner, queryEnv); + CurrentResourceOwner, queryEnv, revalidationResult); INSTR_TIME_SET_CURRENT(planduration); INSTR_TIME_SUBTRACT(planduration, planstart); diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 6b28e64f024..265ec7b9420 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -1657,7 +1657,7 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan, */ /* Replan if needed, and increment plan refcount for portal */ - cplan = GetCachedPlan(plansource, paramLI, NULL, _SPI_current->queryEnv); + cplan = RevalidateAndGetCachedPlan(plansource, paramLI, NULL, _SPI_current->queryEnv); stmt_list = cplan->stmt_list; if (!plan->saved) @@ -2094,9 +2094,9 @@ SPI_plan_get_cached_plan(SPIPlanPtr plan) error_context_stack = &spierrcontext; /* Get the generic plan for the query */ - cplan = GetCachedPlan(plansource, NULL, - plan->saved ? CurrentResourceOwner : NULL, - _SPI_current->queryEnv); + cplan = RevalidateAndGetCachedPlan(plansource, NULL, + plan->saved ? CurrentResourceOwner : NULL, + _SPI_current->queryEnv); Assert(cplan == plansource->gplan); /* Pop the error context stack */ @@ -2253,6 +2253,8 @@ _SPI_prepare_plan(const char *src, SPIPlanPtr plan) */ plansource = CreateCachedPlan(parsetree, src, + plan->argtypes, + plan->nargs, CreateCommandTag(parsetree->stmt)); /* @@ -2550,8 +2552,8 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, * Replan if needed, and increment plan refcount. If it's a saved * plan, the refcount must be backed by the plan_owner. */ - cplan = GetCachedPlan(plansource, options->params, - plan_owner, _SPI_current->queryEnv); + cplan = RevalidateAndGetCachedPlan(plansource, options->params, + plan_owner, _SPI_current->queryEnv); stmt_list = cplan->stmt_list; diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index f4edb31a0e5..a4d02d222d8 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -1521,6 +1521,7 @@ exec_parse_message(const char *query_string, /* string to execute */ * needs to see the unmodified raw parse tree. */ psrc = CreateCachedPlan(raw_parse_tree, query_string, + paramTypes, numParams, CreateCommandTag(raw_parse_tree->stmt)); /* @@ -1552,6 +1553,7 @@ exec_parse_message(const char *query_string, /* string to execute */ /* Empty input string. This is legal. */ raw_parse_tree = NULL; psrc = CreateCachedPlan(raw_parse_tree, query_string, + paramTypes, numParams, CMDTAG_UNKNOWN); querytree_list = NIL; } @@ -2026,7 +2028,7 @@ exec_bind_message(StringInfo input_message) * will be generated in MessageContext. The plan refcount will be * assigned to the Portal, so it will be released at portal destruction. */ - cplan = GetCachedPlan(psrc, params, NULL, NULL); + cplan = RevalidateAndGetCachedPlan(psrc, params, NULL, NULL); /* * Now we can define the portal. @@ -2636,6 +2638,21 @@ exec_describe_statement_message(const char *stmt_name) pstmt = FetchPreparedStatement(stmt_name, true); psrc = pstmt->plansource; + + /* + * Revalidate the cached query, because maybe the argument types + * should be changed due to plan invalidation. This mostly helps for + * cases where the current session did some DDL that invalidated the + * plan or if it changed the search_path. + * + * This approach cannot fully avoid all argument type errors. It's + * still possible that another backend changes one of the tables that + * is used in the prepared statement, in between this backend its + * Describe and Bind calls. This is very unlikely in practice though, + * and at least on the next Describe+Bind iteration the problem would + * not happen. + */ + RevalidateCachedQuery(psrc, NULL); } else { diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c index ee6718dcff3..fd660dae0ea 100644 --- a/src/backend/utils/cache/plancache.c +++ b/src/backend/utils/cache/plancache.c @@ -101,8 +101,6 @@ static dlist_head saved_plan_list = DLIST_STATIC_INIT(saved_plan_list); static dlist_head cached_expression_list = DLIST_STATIC_INIT(cached_expression_list); static void ReleaseGenericPlan(CachedPlanSource *plansource); -static List *RevalidateCachedQuery(CachedPlanSource *plansource, - QueryEnvironment *queryEnv); static bool CheckCachedPlan(CachedPlanSource *plansource); static CachedPlan *BuildCachedPlan(CachedPlanSource *plansource, List *qlist, ParamListInfo boundParams, QueryEnvironment *queryEnv); @@ -167,6 +165,8 @@ InitPlanCache(void) CachedPlanSource * CreateCachedPlan(RawStmt *raw_parse_tree, const char *query_string, + const Oid *param_types, + int num_params, CommandTag commandTag) { CachedPlanSource *plansource; @@ -200,6 +200,16 @@ CreateCachedPlan(RawStmt *raw_parse_tree, plansource->commandTag = commandTag; plansource->param_types = NULL; plansource->num_params = 0; + if (num_params > 0) + { + plansource->orig_param_types = palloc_array(Oid, num_params); + memcpy(plansource->orig_param_types, param_types, num_params * sizeof(Oid)); + } + else + { + plansource->orig_param_types = NULL; + } + plansource->orig_num_params = num_params; plansource->parserSetup = NULL; plansource->parserSetupArg = NULL; plansource->cursor_options = 0; @@ -224,6 +234,7 @@ CreateCachedPlan(RawStmt *raw_parse_tree, plansource->num_generic_plans = 0; plansource->num_custom_plans = 0; + MemoryContextSwitchTo(oldcxt); return plansource; @@ -550,7 +561,7 @@ ReleaseGenericPlan(CachedPlanSource *plansource) * had to do re-analysis, and NIL otherwise. (This is returned just to save * a tree copying step in a subsequent BuildCachedPlan call.) */ -static List * +List * RevalidateCachedQuery(CachedPlanSource *plansource, QueryEnvironment *queryEnv) { @@ -687,11 +698,26 @@ RevalidateCachedQuery(CachedPlanSource *plansource, plansource->parserSetupArg, queryEnv); else - tlist = pg_analyze_and_rewrite_fixedparams(rawtree, - plansource->query_string, - plansource->param_types, - plansource->num_params, - queryEnv); + { + int num_params = plansource->orig_num_params; + Oid *param_types = palloc_array(Oid, num_params); + + memcpy(param_types, plansource->orig_param_types, sizeof(Oid) * num_params); + + /* + * Analyze and rewrite the query. Note that the originally specified + * parameter set is not required to be complete, so we have to use + * pg_analyze_and_rewrite_varparams(). We cannot reuse the previously + * filled in param_types, because the filled in argument types might + * need to change due to the invalidation. + */ + tlist = pg_analyze_and_rewrite_varparams(rawtree, + plansource->query_string, + ¶m_types, + &num_params, + queryEnv); + memcpy(plansource->param_types, param_types, sizeof(Oid) * num_params); + } /* Release snapshot if we got one */ if (snapshot_set) @@ -1120,6 +1146,9 @@ cached_plan_cost(CachedPlan *plan, bool include_planner) * plan or a custom plan for the given parameters: the caller does not know * which it will get. * + * qlist should be the result value from a previous RevalidateCachedQuery, + * or it can be set to NIL if we need to re-copy the plansource's query_list. + * * On return, the plan is valid and we have sufficient locks to begin * execution. * @@ -1132,11 +1161,13 @@ cached_plan_cost(CachedPlan *plan, bool include_planner) * is used for that work. */ CachedPlan * -GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams, - ResourceOwner owner, QueryEnvironment *queryEnv) +GetCachedPlan(CachedPlanSource *plansource, + ParamListInfo boundParams, + ResourceOwner owner, + QueryEnvironment *queryEnv, + List *qlist) { CachedPlan *plan = NULL; - List *qlist; bool customplan; /* Assert caller is doing things in a sane order */ @@ -1146,9 +1177,6 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams, if (owner && !plansource->is_saved) elog(ERROR, "cannot apply ResourceOwner to non-saved cached plan"); - /* Make sure the querytree list is valid and we have parse-time locks */ - qlist = RevalidateCachedQuery(plansource, queryEnv); - /* Decide whether to use a custom plan */ customplan = choose_custom_plan(plansource, boundParams); @@ -1243,6 +1271,31 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams, return plan; } +/* + * RevalidateAndGetCachedPlan: get a cached plan from a CachedPlanSource. + * + * This function is the same as GetCachedPlan, except that it handles + * revalidating the query. + */ +CachedPlan * +RevalidateAndGetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams, + ResourceOwner owner, QueryEnvironment *queryEnv) +{ + List *qlist; + + /* Assert caller is doing things in a sane order */ + Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC); + Assert(plansource->is_complete); + /* This seems worth a real test, though */ + if (owner && !plansource->is_saved) + elog(ERROR, "cannot apply ResourceOwner to non-saved cached plan"); + + /* Make sure the querytree list is valid and we have parse-time locks */ + qlist = RevalidateCachedQuery(plansource, queryEnv); + + return GetCachedPlan(plansource, boundParams, owner, queryEnv, qlist); +} + /* * ReleaseCachedPlan: release active use of a cached plan. * diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h index 36fd0104aec..5d41b52e020 100644 --- a/src/include/utils/plancache.h +++ b/src/include/utils/plancache.h @@ -99,8 +99,25 @@ typedef struct CachedPlanSource struct RawStmt *raw_parse_tree; /* output of raw_parser(), or NULL */ const char *query_string; /* source text of query */ CommandTag commandTag; /* 'nuff said */ + + /* + * We store two versions of the parameter types: + * + * 1. The ones originally given when creating the prepared statement. + * These might contain unresolved types, e.g. if unknown is given as a + * type. + */ + Oid *orig_param_types; /* array of original parameter type OIDs, + * or NULL */ + int orig_num_params; /* length of orig_param_types array */ + + /* + * 2. The ones we actually use for execution. These are resolved versions + * of the original types based on the actual query and underlying objects. + */ Oid *param_types; /* array of parameter type OIDs, or NULL */ int num_params; /* length of param_types array */ + ParserSetupHook parserSetup; /* alternative parameter spec method */ void *parserSetupArg; int cursor_options; /* cursor options used for planning */ @@ -189,6 +206,8 @@ extern void ResetPlanCache(void); extern CachedPlanSource *CreateCachedPlan(struct RawStmt *raw_parse_tree, const char *query_string, + const Oid *param_types, + int num_params, CommandTag commandTag); extern CachedPlanSource *CreateOneShotCachedPlan(struct RawStmt *raw_parse_tree, const char *query_string, @@ -218,7 +237,14 @@ extern List *CachedPlanGetTargetList(CachedPlanSource *plansource, extern CachedPlan *GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams, ResourceOwner owner, - QueryEnvironment *queryEnv); + QueryEnvironment *queryEnv, + List *qlist); +extern CachedPlan *RevalidateAndGetCachedPlan(CachedPlanSource *plansource, + ParamListInfo boundParams, + ResourceOwner owner, + QueryEnvironment *queryEnv); +extern List *RevalidateCachedQuery(CachedPlanSource *plansource, + QueryEnvironment *queryEnv); extern void ReleaseCachedPlan(CachedPlan *plan, ResourceOwner owner); extern bool CachedPlanAllowsSimpleValidityCheck(CachedPlanSource *plansource, diff --git a/src/test/regress/expected/plancache.out b/src/test/regress/expected/plancache.out index 77ae17b074a..b464ea4e448 100644 --- a/src/test/regress/expected/plancache.out +++ b/src/test/regress/expected/plancache.out @@ -15,7 +15,7 @@ EXECUTE prepstmt; (5 rows) -- and one with parameters -PREPARE prepstmt2(bigint) AS SELECT * FROM pcachetest WHERE q1 = $1; +PREPARE prepstmt2(unknown) AS SELECT * FROM pcachetest WHERE q1 = $1; EXECUTE prepstmt2(123); q1 | q2 -----+------------------ @@ -114,6 +114,34 @@ EXECUTE vprep; 4567890123456789 | 2283945061728394 (5 rows) +DROP VIEW pcacheview; +-- When using unknown as an argument type in a prpared statement, the resolved +-- argument types can change due to DDL or search_path changes. +ALTER TABLE pcachetest ALTER COLUMN q1 TYPE text; +EXECUTE prepstmt2('123'); + q1 | q2 +-----+------------------ + 123 | 456 + 123 | 4567890123456789 +(2 rows) + +EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE prepstmt2('123'); + QUERY PLAN +------------------------------------------------ + Seq Scan on pcachetest (actual rows=2 loops=1) + Filter: (q1 = $1) + Rows Removed by Filter: 3 +(3 rows) + +ALTER TABLE pcachetest ALTER COLUMN q1 TYPE bigint USING q1::bigint; +EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE prepstmt2(123); + QUERY PLAN +------------------------------------------------ + Seq Scan on pcachetest (actual rows=2 loops=1) + Filter: (q1 = $1) + Rows Removed by Filter: 3 +(3 rows) + -- Check basic SPI plan invalidation create function cache_test(int) returns int as $$ declare total int; diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql index cab0c42a142..9a707458606 100644 --- a/src/test/regress/sql/plancache.sql +++ b/src/test/regress/sql/plancache.sql @@ -10,7 +10,7 @@ PREPARE prepstmt AS SELECT * FROM pcachetest; EXECUTE prepstmt; -- and one with parameters -PREPARE prepstmt2(bigint) AS SELECT * FROM pcachetest WHERE q1 = $1; +PREPARE prepstmt2(unknown) AS SELECT * FROM pcachetest WHERE q1 = $1; EXECUTE prepstmt2(123); @@ -53,6 +53,16 @@ CREATE OR REPLACE TEMP VIEW pcacheview AS EXECUTE vprep; +DROP VIEW pcacheview; +-- When using unknown as an argument type in a prpared statement, the resolved +-- argument types can change due to DDL or search_path changes. +ALTER TABLE pcachetest ALTER COLUMN q1 TYPE text; +EXECUTE prepstmt2('123'); + +EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE prepstmt2('123'); +ALTER TABLE pcachetest ALTER COLUMN q1 TYPE bigint USING q1::bigint; +EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE prepstmt2(123); + -- Check basic SPI plan invalidation create function cache_test(int) returns int as $$ -- 2.34.1