Re: Centralizing protective copying of utility statements - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: Centralizing protective copying of utility statements |
Date | |
Msg-id | 1026052.1623949409@sss.pgh.pa.us Whole thread Raw |
In response to | Re: Centralizing protective copying of utility statements (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: Centralizing protective copying of utility statements
Re: Centralizing protective copying of utility statements |
List | pgsql-hackers |
I wrote: > What seems like a much safer answer is to make the API change noisy. > That is, move the responsibility for actually calling copyObject > into ProcessUtility itself, and add a bool parameter saying whether > that needs to be done. That forces all callers to consider the > issue, and gives them the tool they need to make the right thing > happen. Here's a v2 that does it like that. In this formulation, we're basically hoisting the responsibility for doing copyObject up into ProcessUtility from its direct children, which seems like a clearer way of thinking about what has to change. We could avoid the side-effects on users of ProcessUtility_hook by doing the copy step in ProcessUtility itself rather than passing the flag on to standard_ProcessUtility. But that sounded like a bit of a kluge. Also, putting the work in standard_ProcessUtility preserves the option to redistribute it into the individual switch arms, in case anyone does find the extra copying overhead annoying for statement types that don't need it. (I don't plan to do any such thing as part of this bug-fix patch, though.) Barring objections, I'm going to push this into HEAD fairly soon, since beta2 is hard upon us. Still thinking about which way to fix it in the back branches. regards, tom lane diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 09433c8c96..07fe0e7cda 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -320,6 +320,7 @@ static void pgss_ExecutorRun(QueryDesc *queryDesc, static void pgss_ExecutorFinish(QueryDesc *queryDesc); static void pgss_ExecutorEnd(QueryDesc *queryDesc); static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, + bool readOnlyTree, ProcessUtilityContext context, ParamListInfo params, QueryEnvironment *queryEnv, DestReceiver *dest, QueryCompletion *qc); @@ -1069,6 +1070,7 @@ pgss_ExecutorEnd(QueryDesc *queryDesc) */ static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, + bool readOnlyTree, ProcessUtilityContext context, ParamListInfo params, QueryEnvironment *queryEnv, DestReceiver *dest, QueryCompletion *qc) @@ -1126,11 +1128,11 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, PG_TRY(); { if (prev_ProcessUtility) - prev_ProcessUtility(pstmt, queryString, + prev_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); else - standard_ProcessUtility(pstmt, queryString, + standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); } @@ -1176,11 +1178,11 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString, else { if (prev_ProcessUtility) - prev_ProcessUtility(pstmt, queryString, + prev_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); else - standard_ProcessUtility(pstmt, queryString, + standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); } diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c index 34de6158d6..19a3ffb7ff 100644 --- a/contrib/sepgsql/hooks.c +++ b/contrib/sepgsql/hooks.c @@ -313,6 +313,7 @@ sepgsql_exec_check_perms(List *rangeTabls, bool abort) static void sepgsql_utility_command(PlannedStmt *pstmt, const char *queryString, + bool readOnlyTree, ProcessUtilityContext context, ParamListInfo params, QueryEnvironment *queryEnv, @@ -378,11 +379,11 @@ sepgsql_utility_command(PlannedStmt *pstmt, } if (next_ProcessUtility_hook) - (*next_ProcessUtility_hook) (pstmt, queryString, + (*next_ProcessUtility_hook) (pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); else - standard_ProcessUtility(pstmt, queryString, + standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); } diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index 89a4f8f810..b6eacd5baa 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -438,14 +438,8 @@ BeginCopyTo(ParseState *pstate, /* * Run parse analysis and rewrite. Note this also acquires sufficient * locks on the source table(s). - * - * Because the parser and planner tend to scribble on their input, we - * make a preliminary copy of the source querytree. This prevents - * problems in the case that the COPY is in a portal or plpgsql - * function and is executed repeatedly. (See also the same hack in - * DECLARE CURSOR and PREPARE.) XXX FIXME someday. */ - rewritten = pg_analyze_and_rewrite(copyObject(raw_query), + rewritten = pg_analyze_and_rewrite(raw_query, pstate->p_sourcetext, NULL, 0, NULL); diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c index dce882012e..0982851715 100644 --- a/src/backend/commands/createas.c +++ b/src/backend/commands/createas.c @@ -299,14 +299,8 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt, * rewriter. We do not do AcquireRewriteLocks: we assume the query * either came straight from the parser, or suitable locks were * acquired by plancache.c. - * - * Because the rewriter and planner tend to scribble on the input, we - * make a preliminary copy of the source querytree. This prevents - * problems in the case that CTAS is in a portal or plpgsql function - * and is executed repeatedly. (See also the same hack in EXPLAIN and - * PREPARE.) */ - rewritten = QueryRewrite(copyObject(query)); + rewritten = QueryRewrite(query); /* SELECT should never rewrite to more or less than one SELECT query */ if (list_length(rewritten) != 1) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index 9a60865d19..e81b990092 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -256,14 +256,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, * rewriter. We do not do AcquireRewriteLocks: we assume the query either * came straight from the parser, or suitable locks were acquired by * plancache.c. - * - * Because the rewriter and planner tend to scribble on the input, we make - * a preliminary copy of the source querytree. This prevents problems in - * the case that the EXPLAIN is in a portal or plpgsql function and is - * executed repeatedly. (See also the same hack in DECLARE CURSOR and - * PREPARE.) XXX FIXME someday. */ - rewritten = QueryRewrite(castNode(Query, copyObject(stmt->query))); + rewritten = QueryRewrite(castNode(Query, stmt->query)); /* emit opening boilerplate */ ExplainBeginOutput(es); @@ -427,7 +421,8 @@ ExplainOneQuery(Query *query, int cursorOptions, * "into" is NULL unless we are explaining the contents of a CreateTableAsStmt. * * This is exported because it's called back from prepare.c in the - * EXPLAIN EXECUTE case. + * EXPLAIN EXECUTE case. In that case, we'll be dealing with a statement + * that's in the plan cache, so we have to ensure we don't modify it. */ void ExplainOneUtility(Node *utilityStmt, IntoClause *into, ExplainState *es, @@ -441,8 +436,7 @@ ExplainOneUtility(Node *utilityStmt, IntoClause *into, ExplainState *es, { /* * We have to rewrite the contained SELECT and then pass it back to - * ExplainOneQuery. It's probably not really necessary to copy the - * contained parsetree another time, but let's be safe. + * ExplainOneQuery. Copy to be safe in the EXPLAIN EXECUTE case. */ CreateTableAsStmt *ctas = (CreateTableAsStmt *) utilityStmt; List *rewritten; diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c index 008505368c..41857feda9 100644 --- a/src/backend/commands/extension.c +++ b/src/backend/commands/extension.c @@ -786,6 +786,7 @@ execute_sql_string(const char *sql) ProcessUtility(stmt, sql, + false, PROCESS_UTILITY_QUERY, NULL, NULL, diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c index eb7103fd3b..bc36311d38 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -1570,8 +1570,7 @@ ImportForeignSchema(ImportForeignSchemaStmt *stmt) pstmt->stmt_len = rs->stmt_len; /* Execute statement */ - ProcessUtility(pstmt, - cmd, + ProcessUtility(pstmt, cmd, false, PROCESS_UTILITY_SUBCOMMAND, NULL, NULL, None_Receiver, NULL); diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index 5cacc088cd..5d469309ce 100644 --- a/src/backend/commands/policy.c +++ b/src/backend/commands/policy.c @@ -747,12 +747,12 @@ CreatePolicy(CreatePolicyStmt *stmt) addNSItemToQuery(with_check_pstate, nsitem, false, true, true); qual = transformWhereClause(qual_pstate, - copyObject(stmt->qual), + stmt->qual, EXPR_KIND_POLICY, "POLICY"); with_check_qual = transformWhereClause(with_check_pstate, - copyObject(stmt->with_check), + stmt->with_check, EXPR_KIND_POLICY, "POLICY"); @@ -922,7 +922,7 @@ AlterPolicy(AlterPolicyStmt *stmt) addNSItemToQuery(qual_pstate, nsitem, false, true, true); - qual = transformWhereClause(qual_pstate, copyObject(stmt->qual), + qual = transformWhereClause(qual_pstate, stmt->qual, EXPR_KIND_POLICY, "POLICY"); @@ -946,7 +946,7 @@ AlterPolicy(AlterPolicyStmt *stmt) addNSItemToQuery(with_check_pstate, nsitem, false, true, true); with_check_qual = transformWhereClause(with_check_pstate, - copyObject(stmt->with_check), + stmt->with_check, EXPR_KIND_POLICY, "POLICY"); diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c index d34cc39fde..3ea30bcbc9 100644 --- a/src/backend/commands/portalcmds.c +++ b/src/backend/commands/portalcmds.c @@ -76,14 +76,8 @@ PerformCursorOpen(ParseState *pstate, DeclareCursorStmt *cstmt, ParamListInfo pa * rewriter. We do not do AcquireRewriteLocks: we assume the query either * came straight from the parser, or suitable locks were acquired by * plancache.c. - * - * Because the rewriter and planner tend to scribble on the input, we make - * a preliminary copy of the source querytree. This prevents problems in - * the case that the DECLARE CURSOR is in a portal or plpgsql function and - * is executed repeatedly. (See also the same hack in EXPLAIN and - * PREPARE.) XXX FIXME someday. */ - rewritten = QueryRewrite((Query *) copyObject(query)); + rewritten = QueryRewrite(query); /* SELECT should never rewrite to more or less than one query */ if (list_length(rewritten) != 1) diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c index 65d8ac65b1..5e03c7c5aa 100644 --- a/src/backend/commands/prepare.c +++ b/src/backend/commands/prepare.c @@ -78,12 +78,9 @@ PrepareQuery(ParseState *pstate, PrepareStmt *stmt, /* * Need to wrap the contained statement in a RawStmt node to pass it to * parse analysis. - * - * Because parse analysis scribbles on the raw querytree, we must make a - * copy to ensure we don't modify the passed-in tree. FIXME someday. */ rawstmt = makeNode(RawStmt); - rawstmt->stmt = (Node *) copyObject(stmt->query); + rawstmt->stmt = stmt->query; rawstmt->stmt_location = stmt_location; rawstmt->stmt_len = stmt_len; diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c index a60eb161e4..6c6ab9ee34 100644 --- a/src/backend/commands/schemacmds.c +++ b/src/backend/commands/schemacmds.c @@ -191,6 +191,7 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString, /* do this step */ ProcessUtility(wrapper, queryString, + false, PROCESS_UTILITY_SUBCOMMAND, NULL, NULL, diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 028e8ac46b..4e23c7fce5 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -4408,8 +4408,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, * Copy the original subcommand for each table. This avoids conflicts * when different child tables need to make different parse * transformations (for example, the same column may have different column - * numbers in different children). It also ensures that we don't corrupt - * the original parse tree, in case it is saved in plancache. + * numbers in different children). */ cmd = copyObject(cmd); diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c index f2642dba6c..4df05a0b33 100644 --- a/src/backend/commands/view.c +++ b/src/backend/commands/view.c @@ -417,12 +417,9 @@ DefineView(ViewStmt *stmt, const char *queryString, /* * Run parse analysis to convert the raw parse tree to a Query. Note this * also acquires sufficient locks on the source table(s). - * - * Since parse analysis scribbles on its input, copy the raw parse tree; - * this ensures we don't corrupt a prepared statement, for example. */ rawstmt = makeNode(RawStmt); - rawstmt->stmt = (Node *) copyObject(stmt->query); + rawstmt->stmt = stmt->query; rawstmt->stmt_location = stmt_location; rawstmt->stmt_len = stmt_len; diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c index e2ea51aafe..296e54e60a 100644 --- a/src/backend/executor/functions.c +++ b/src/backend/executor/functions.c @@ -886,6 +886,7 @@ postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache) { ProcessUtility(es->qd->plannedstmt, fcache->src, + false, PROCESS_UTILITY_QUERY, es->qd->params, es->qd->queryEnv, diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index b8bd05e894..c0a4f58f02 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -2545,6 +2545,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI, InitializeQueryCompletion(&qc); ProcessUtility(stmt, plansource->query_string, + true, context, paramLI, _SPI_current->queryEnv, diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index c9708e38f4..81d3e7990c 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -11,10 +11,6 @@ * Hence these functions are now called at the start of execution of their * respective utility commands. * - * NOTE: in general we must avoid scribbling on the passed-in raw parse - * tree, since it might be in a plan cache. The simplest solution is - * a quick copyObject() call before manipulating the query tree. - * * * Portions Copyright (c) 1996-2021, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California @@ -177,12 +173,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) Oid existing_relid; ParseCallbackState pcbstate; - /* - * We must not scribble on the passed-in CreateStmt, so copy it. (This is - * overkill, but easy.) - */ - stmt = copyObject(stmt); - /* Set up pstate */ pstate = make_parsestate(NULL); pstate->p_sourcetext = queryString; @@ -2824,12 +2814,6 @@ transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString) if (stmt->transformed) return stmt; - /* - * We must not scribble on the passed-in IndexStmt, so copy it. (This is - * overkill, but easy.) - */ - stmt = copyObject(stmt); - /* Set up pstate */ pstate = make_parsestate(NULL); pstate->p_sourcetext = queryString; @@ -2925,12 +2909,6 @@ transformStatsStmt(Oid relid, CreateStatsStmt *stmt, const char *queryString) if (stmt->transformed) return stmt; - /* - * We must not scribble on the passed-in CreateStatsStmt, so copy it. - * (This is overkill, but easy.) - */ - stmt = copyObject(stmt); - /* Set up pstate */ pstate = make_parsestate(NULL); pstate->p_sourcetext = queryString; @@ -2993,9 +2971,6 @@ transformStatsStmt(Oid relid, CreateStatsStmt *stmt, const char *queryString) * * actions and whereClause are output parameters that receive the * transformed results. - * - * Note that we must not scribble on the passed-in RuleStmt, so we do - * copyObject() on the actions and WHERE clause. */ void transformRuleStmt(RuleStmt *stmt, const char *queryString, @@ -3070,7 +3045,7 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString, /* take care of the where clause */ *whereClause = transformWhereClause(pstate, - (Node *) copyObject(stmt->whereClause), + stmt->whereClause, EXPR_KIND_WHERE, "WHERE"); /* we have to fix its collations too */ @@ -3142,8 +3117,7 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString, addNSItemToQuery(sub_pstate, newnsitem, false, true, false); /* Transform the rule action statement */ - top_subqry = transformStmt(sub_pstate, - (Node *) copyObject(action)); + top_subqry = transformStmt(sub_pstate, action); /* * We cannot support utility-statement actions (eg NOTIFY) with @@ -3325,12 +3299,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, AlterTableCmd *newcmd; ParseNamespaceItem *nsitem; - /* - * We must not scribble on the passed-in AlterTableStmt, so copy it. (This - * is overkill, but easy.) - */ - stmt = copyObject(stmt); - /* Caller is responsible for locking the relation */ rel = relation_open(relid, NoLock); tupdesc = RelationGetDescr(rel); diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c index f7f08e6c05..920ac50baf 100644 --- a/src/backend/tcop/pquery.c +++ b/src/backend/tcop/pquery.c @@ -1144,8 +1144,13 @@ PortalRunUtility(Portal portal, PlannedStmt *pstmt, else portal->portalSnapshot = NULL; + /* + * Run it. We tell ProcessUtility that the node tree is read-only if it + * came from a cached plan. + */ ProcessUtility(pstmt, portal->sourceText, + (portal->cplan != NULL), isTopLevel ? PROCESS_UTILITY_TOPLEVEL : PROCESS_UTILITY_QUERY, portal->portalParams, portal->queryEnv, diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 1a8fc16773..d00189fe32 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -476,6 +476,7 @@ CheckRestrictedOperation(const char *cmdname) * * pstmt: PlannedStmt wrapper for the utility statement * queryString: original source text of command + * readOnlyTree: treat pstmt's node tree as read-only * context: identifies source of statement (toplevel client command, * non-toplevel client command, subcommand of a larger utility command) * params: parameters to use during execution @@ -501,6 +502,7 @@ CheckRestrictedOperation(const char *cmdname) void ProcessUtility(PlannedStmt *pstmt, const char *queryString, + bool readOnlyTree, ProcessUtilityContext context, ParamListInfo params, QueryEnvironment *queryEnv, @@ -518,11 +520,11 @@ ProcessUtility(PlannedStmt *pstmt, * call standard_ProcessUtility(). */ if (ProcessUtility_hook) - (*ProcessUtility_hook) (pstmt, queryString, + (*ProcessUtility_hook) (pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); else - standard_ProcessUtility(pstmt, queryString, + standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); } @@ -541,13 +543,14 @@ ProcessUtility(PlannedStmt *pstmt, void standard_ProcessUtility(PlannedStmt *pstmt, const char *queryString, + bool readOnlyTree, ProcessUtilityContext context, ParamListInfo params, QueryEnvironment *queryEnv, DestReceiver *dest, QueryCompletion *qc) { - Node *parsetree = pstmt->utilityStmt; + Node *parsetree; bool isTopLevel = (context == PROCESS_UTILITY_TOPLEVEL); bool isAtomicContext = (!(context == PROCESS_UTILITY_TOPLEVEL || context == PROCESS_UTILITY_QUERY_NONATOMIC)|| IsTransactionBlock()); ParseState *pstate; @@ -556,6 +559,18 @@ standard_ProcessUtility(PlannedStmt *pstmt, /* This can recurse, so check for excessive recursion */ check_stack_depth(); + /* + * If the given node tree is read-only, make a copy to ensure that parse + * transformations don't damage the original tree. This could be + * refactored to avoid making unnecessary copies in more cases, but it's + * not clear that it's worth a great deal of trouble over. Statements + * that are complex enough to be expensive to copy are exactly the ones + * we'd need to copy, so that only marginal savings seem possible. + */ + if (readOnlyTree) + pstmt = copyObject(pstmt); + parsetree = pstmt->utilityStmt; + /* Prohibit read/write commands in read-only states. */ readonly_flags = ClassifyUtilityCommandAsReadOnly(parsetree); if (readonly_flags != COMMAND_IS_STRICTLY_READ_ONLY && @@ -1211,6 +1226,7 @@ ProcessUtilitySlow(ParseState *pstate, ProcessUtility(wrapper, queryString, + false, PROCESS_UTILITY_SUBCOMMAND, params, NULL, @@ -1918,6 +1934,7 @@ ProcessUtilityForAlterTable(Node *stmt, AlterTableUtilityContext *context) ProcessUtility(wrapper, context->queryString, + false, PROCESS_UTILITY_SUBCOMMAND, context->params, context->queryEnv, diff --git a/src/include/tcop/utility.h b/src/include/tcop/utility.h index 841062b4b3..212e9b3280 100644 --- a/src/include/tcop/utility.h +++ b/src/include/tcop/utility.h @@ -69,17 +69,21 @@ typedef struct AlterTableUtilityContext /* Hook for plugins to get control in ProcessUtility() */ typedef void (*ProcessUtility_hook_type) (PlannedStmt *pstmt, - const char *queryString, ProcessUtilityContext context, + const char *queryString, + bool readOnlyTree, + ProcessUtilityContext context, ParamListInfo params, QueryEnvironment *queryEnv, DestReceiver *dest, QueryCompletion *qc); extern PGDLLIMPORT ProcessUtility_hook_type ProcessUtility_hook; extern void ProcessUtility(PlannedStmt *pstmt, const char *queryString, + bool readOnlyTree, ProcessUtilityContext context, ParamListInfo params, QueryEnvironment *queryEnv, DestReceiver *dest, QueryCompletion *qc); extern void standard_ProcessUtility(PlannedStmt *pstmt, const char *queryString, + bool readOnlyTree, ProcessUtilityContext context, ParamListInfo params, QueryEnvironment *queryEnv, DestReceiver *dest, QueryCompletion *qc);
pgsql-hackers by date: