8000 Centralize the logic for protective copying of utility statements. · postgrespro/postgres@7c337b6 · GitHub
[go: up one dir, main page]
Skip to content

Commit 7c337b6

Browse files
tglsfdctglsfdc
committed
Centralize the logic for protective copying of utility statements.
In the "simple Query" code path, it's fine for parse analysis or execution of a utility statement to scribble on the statement's node tree, since that'll just be thrown away afterwards. However it's not fine if the node tree is in the plan cache, as then it'd be corrupted for subsequent executions. Up to now we've dealt with that by having individual utility-statement functions apply copyObject() if they were going to modify the tree. But that's prone to errors of omission. Bug #17053 from Charles Samborski shows that CREATE/ALTER DOMAIN didn't get this memo, and can crash if executed repeatedly from plan cache. In the back branches, we'll just apply a narrow band-aid for that, but in HEAD it seems prudent to have a more principled fix that will close off the possibility of other similar bugs in future. Hence, let's hoist the responsibility for doing copyObject up into ProcessUtility from its children, thus ensuring that it happens for all utility statement types. Also, modify ProcessUtility's API so that its callers can tell it whether a copy step is necessary. It turns out that in all cases, the immediate caller knows whether the node tree is transient, so this doesn't involve a huge amount of code thrashing. In this way, while we lose a little bit in the execute-from-cache code path due to sometimes copying node trees that wouldn't be mutated anyway, we gain something in the simple-Query code path by not copying throwaway node trees. Statements that are complex enough to be expensive to copy are almost certainly ones that would have to be copied anyway, so the loss in the cache code path shouldn't be much. (Note that this whole problem applies only to utility statements. Optimizable statements don't have the issue because we long ago made the executor treat Plan trees as read-only. Perhaps someday we will make utility statement execution act likewise, but I'm not holding my breath.) Discussion: https://postgr.es/m/931771.1623893989@sss.pgh.pa.us Discussion: https://postgr.es/m/17053-3ca3f501bbc212b4@postgresql.org
1 parent 0a4efdc commit 7c337b6

File tree

19 files changed

+56
-91
lines changed

19 files changed

+56
-91
lines changed

‎contrib/pg_stat_statements/pg_stat_statements.c

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,7 @@ static void pgss_ExecutorRun(QueryDesc *queryDesc,
320320
static void pgss_ExecutorFinish(QueryDesc *queryDesc);
321321
static void pgss_ExecutorEnd(QueryDesc *queryDesc);
322322
static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
323+
bool readOnlyTree,
323324
ProcessUtilityContext context, ParamListInfo params,
324325
QueryEnvironment *queryEnv,
325326
DestReceiver *dest, QueryCompletion *qc);
@@ -1069,6 +1070,7 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
10691070
*/
10701071
static void
10711072
pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
1073+
bool readOnlyTree,
10721074
ProcessUtilityContext context,
10731075
ParamListInfo params, QueryEnvironment *queryEnv,
10741076
DestReceiver *dest, QueryCompletion *qc)
@@ -1126,11 +1128,11 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
11261128
PG_TRY();
11271129
{
11281130
if (prev_ProcessUtility)
1129-
prev_ProcessUtility(pstmt, queryString,
1131+
prev_ProcessUtility(pstmt, queryString, readOnlyTree,
11301132
context, params, queryEnv,
11311133
dest, qc);
11321134
else
1133-
standard_ProcessUtility(pstmt, queryString,
1135+
standard_ProcessUtility(pstmt, queryString, readOnlyTree,
11341136
context, params, queryEnv,
11351137
dest, qc);
11361138
}
@@ -1176,11 +1178,11 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
11761178
else
11771179
{
11781180
if (prev_ProcessUtility)
1179-
prev_ProcessUtility(pstmt, queryString,
1181+
prev_ProcessUtility(pstmt, queryString, readOnlyTree,
11801182
context, params, queryEnv,
11811183
dest, qc);
11821184
else
1183-
standard_ProcessUtility(pstmt, queryString,
1185+
standard_ProcessUtility(pstmt, queryString, readOnlyTree,
11841186
context, params, queryEnv,
11851187
dest, qc);
11861188
}

‎contrib/sepgsql/hooks.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,7 @@ sepgsql_exec_check_perms(List *rangeTabls, bool abort)
313313
static void
314314
sepgsql_utility_command(PlannedStmt *pstmt,
315315
const char *queryString,
316+
bool readOnlyTree,
316317
ProcessUtilityContext context,
317318
ParamListInfo params,
318319
QueryEnvironment *queryEnv,
@@ -378,11 +379,11 @@ sepgsql_utility_command(PlannedStmt *pstmt,
378379
}
379380

380381
if (next_ProcessUtility_hook)
381-
(*next_ProcessUtility_hook) (pstmt, queryString,
382+
(*next_ProcessUtility_hook) (pstmt, queryString, readOnlyTree,
382383
context, params, queryEnv,
383384
dest, qc);
384385
else
385-
standard_ProcessUtility(pstmt, queryString,
386+
standard_ProcessUtility(pstmt, queryString, readOnlyTree,
386387
context, params, queryEnv,
387388
dest, qc);
388389
}

‎src/backend/commands/copyto.c

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -438,14 +438,8 @@ BeginCopyTo(ParseState *pstate,
438438
/*
439439
* Run parse analysis and rewrite. Note this also acquires sufficient
440440
* locks on the source table(s).
441-
*
442-
* Because the parser and planner tend to scribble on their input, we
443-
* make a preliminary copy of the source querytree. This prevents
444-
* problems in the case that the COPY is in a portal or plpgsql
445-
* function and is executed repeatedly. (See also the same hack in
446-
* DECLARE CURSOR and PREPARE.) XXX FIXME someday.
447441
*/
448-
rewritten = pg_analyze_and_rewrite(copyObject(raw_query),
442+
rewritten = pg_analyze_and_rewrite(raw_query,
449443
pstate->p_sourcetext, NULL, 0,
450444
NULL);
451445

‎src/backend/commands/createas.c

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -299,14 +299,8 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
299299
* rewriter. We do not do AcquireRewriteLocks: we assume the query
300300
* either came straight from the parser, or suitable locks were
301301
* acquired by plancache.c.
302-
*
303-
* Because the rewriter and planner tend to scribble on the input, we
304-
* make a preliminary copy of the source querytree. This prevents
305-
* problems in the case that CTAS is in a portal or plpgsql function
306-
* and is executed repeatedly. (See also the same hack in EXPLAIN and
307-
* PREPARE.)
308302
*/
309-
rewritten = QueryRewrite(copyObject(query));
303+
rewritten = QueryRewrite(query);
310304

311305
/* SELECT should never rewrite to more or less than one SELECT query */
312306
if (list_length(rewritten) != 1)

‎src/backend/commands/explain.c

Lines changed: 4 additions & 10 deletions
< 10000 /tr>
Original file line numberDiff line numberDiff line change
@@ -256,14 +256,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
256256
* rewriter. We do not do AcquireRewriteLocks: we assume the query either
257257
* came straight from the parser, or suitable locks were acquired by
258258
* plancache.c.
259-
*
260-
* Because the rewriter and planner tend to scribble on the input, we make
261-
* a preliminary copy of the source querytree. This prevents problems in
262-
* the case that the EXPLAIN is in a portal or plpgsql function and is
263-
* executed repeatedly. (See also the same hack in DECLARE CURSOR and
264-
* PREPARE.) XXX FIXME someday.
265259
*/
266-
rewritten = QueryRewrite(castNode(Query, copyObject(stmt->query)));
260+
rewritten = QueryRewrite(castNode(Query, stmt->query));
267261

268262
/* emit opening boilerplate */
269263
ExplainBeginOutput(es);
@@ -427,7 +421,8 @@ ExplainOneQuery(Query *query, int cursorOptions,
427421
* "into" is NULL unless we are explaining the contents of a CreateTableAsStmt.
428422
*
429423
* This is exported because it's called back from prepare.c in the
430-
* EXPLAIN EXECUTE case.
424+
* EXPLAIN EXECUTE case. In that case, we'll be dealing with a statement
425+
* that's in the plan cache, so we have to ensure we don't modify it.
431426
*/
432427
void
433428
ExplainOneUtility(Node *utilityStmt, IntoClause *into, ExplainState *es,
@@ -441,8 +436,7 @@ ExplainOneUtility(Node *utilityStmt, IntoClause *into, ExplainState *es,
441436
{
442437
/*
443438
* We have to rewrite the contained SELECT and then pass it back to
444-
* ExplainOneQuery. It's probably not really necessary to copy the
445-
* contained parsetree another time, but let's be safe.
439+
* ExplainOneQuery. Copy to be safe in the EXPLAIN EXECUTE case.
446440
*/
447441
CreateTableAsStmt *ctas = (CreateTableAsStmt *) utilityStmt;
448442
List *rewritten;

‎src/backend/commands/extension.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -786,6 +786,7 @@ execute_sql_string(const char *sql)
786786

787787
ProcessUtility(stmt,
788788
sql,
789+
false,
789790
PROCESS_UTILITY_QUERY,
790791
NULL,
791792
NULL,

‎src/backend/commands/foreigncmds.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1570,8 +1570,7 @@ ImportForeignSchema(ImportForeignSchemaStmt *stmt)
15701570
pstmt->stmt_len = rs->stmt_len;
15711571

15721572
/* Execute statement */
1573-
ProcessUtility(pstmt,
1574-
cmd,
1573+
ProcessUtility(pstmt, cmd, false,
15751574
PROCESS_UTILITY_SUBCOMMAND, NULL, NULL,
15761575
None_Receiver, NULL);
15771576

‎src/backend/commands/policy.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -747,12 +747,12 @@ CreatePolicy(CreatePolicyStmt *stmt)
747747
addNSItemToQuery(with_check_pstate, nsitem, false, true, true);
748748

749749
qual = transformWhereClause(qual_pstate,
750-
copyObject(stmt->qual),
750+
stmt->qual,
751751
EXPR_KIND_POLICY,
752752
"POLICY");
753753

754754
with_check_qual = transformWhereClause(with_check_pstate,
755-
copyObject(stmt->with_check),
755+
stmt->with_check,
756756
EXPR_KIND_POLICY,
757757
"POLICY");
758758

@@ -922,7 +922,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
922922

923923
addNSItemToQuery(qual_pstate, nsitem, false, true, true);
924924

925-
qual = transformWhereClause(qual_pstate, copyObject(stmt->qual),
925+
qual = transformWhereClause(qual_pstate, stmt->qual,
926926
EXPR_KIND_POLICY,
927927
"POLICY");
928928

@@ -946,7 +946,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
946946
addNSItemToQuery(with_check_pstate, nsitem, false, true, true);
947947

948948
with_check_qual = transformWhereClause(with_check_pstate,
949-
copyObject(stmt->with_check),
949+
stmt->with_check,
950950
EXPR_KIND_POLICY,
951951
"POLICY");
952952

‎src/backend/commands/portalcmds.c

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,14 +76,8 @@ PerformCursorOpen(ParseState *pstate, DeclareCursorStmt *cstmt, ParamListInfo pa
7676
* rewriter. We do not do AcquireRewriteLocks: we assume the query either
7777
* came straight from the parser, or suitable locks were acquired by
7878
* plancache.c.
79-
*
80-
* Because the rewriter and planner tend to scribble on the input, we make
81-
* a preliminary copy of the source querytree. This prevents problems in
82-
* the case that the DECLARE CURSOR is in a portal or plpgsql function and
83-
* is executed repeatedly. (See also the same hack in EXPLAIN and
84-
* PREPARE.) XXX FIXME someday.
8579
*/
86-
rewritten = QueryRewrite((Query *) copyObject(query));
80+
rewritten = QueryRewrite(query);
8781

8882
/* SELECT should never rewrite to more or less than one query */
8983
if (list_length(rewritten) != 1)

‎src/backend/commands/prepare.c

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,9 @@ PrepareQuery(ParseState *pstate, PrepareStmt *stmt,
7878
/*
7979
* Need to wrap the contained statement in a RawStmt node to pass it to
8080
* parse analysis.
81-
*
82-
* Because parse analysis scribbles on the raw querytree, we must make a
83-
* copy to ensure we don't modify the passed-in tree. FIXME someday.
8481
*/
8582
rawstmt = makeNode(RawStmt);
86-
rawstmt->stmt = (Node *) copyObject(stmt->query);
83+
rawstmt->stmt = stmt->query;
8784
rawstmt->stmt_location = stmt_location;
8885
rawstmt->stmt_len = stmt_len;
8986

0 commit comments

Comments
 (0)
0