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://firstname.lastname@example.org Discussion: https://email@example.com
7c337b6b52 Centralize the logic for protective copying of utility statements.
contrib/pg_stat_statements/pg_stat_statements.c | 10 ++++---
contrib/sepgsql/hooks.c | 5 ++--
src/backend/commands/copyto.c | 8 +-----
src/backend/commands/createas.c | 8 +-----
src/backend/commands/explain.c | 14 +++-------
src/backend/commands/extension.c | 1 +
src/backend/commands/foreigncmds.c | 3 +--
src/backend/commands/policy.c | 8 +++---
src/backend/commands/portalcmds.c | 8 +-----
src/backend/commands/prepare.c | 5 +---
src/backend/commands/schemacmds.c | 1 +
src/backend/commands/tablecmds.c | 3 +--
src/backend/commands/view.c | 5 +---
src/backend/executor/functions.c | 1 +
src/backend/executor/spi.c | 1 +
src/backend/parser/parse_utilcmd.c | 36 ++-----------------------
src/backend/tcop/pquery.c | 1 +
src/backend/tcop/utility.c | 23 +++++++++++++---
src/include/tcop/utility.h | 6 ++++-
19 files changed, 56 insertions(+), 91 deletions(-)