Re: Violation of principle that plan trees are read-only

Hi,

On 2025-05-20 13:04:46 -0400, Tom Lane wrote:
> I wrote:
> > I think we can just delete this assignment (and fix these comments).
>
> As attached.

Largely makes sense, the only thing I see is that the !returningLists branch
does:
/*
* We still must construct a dummy result tuple type, because InitPlan
* expects one (maybe should change that?).
*/
mtstate->ps.plan->targetlist = NIL;

which we presumably shouldn't do anymore either. It never changes anything
afaict, but still.

> I'm tempted to back-patch this: the plan tree damage seems harmless at
> present, but maybe it'd become less harmless with future fixes.

I am not sure, I can see arguments either way. <ponder>

There are *some* cases where this changes the explain output, but but the new
output is more correct, I think:

--- /tmp/a.out 2025-05-20 14:19:04.947945026 -0400
+++ /tmp/b.out 2025-05-20 14:19:12.878975702 -0400
@@ -18,7 +18,7 @@
QUERY PLAN
-----------------------------------------------------------------------------
Update on public.part_abc
- Output: part_abc_1.a, part_abc_1.b, part_abc_1.c
+ Output: a, b, c
Update on public.part_abc_2 part_abc_1
-> Append
Subplans Removed: 1

I suspect this is an argument for backpatching, not against - seems that
deparsing could end up creating bogus output in cases where it could matter?
Not sure if such cases are reachable via views (and thus pg_dump) or
postgres_fdw, but it seems possible.

Greetings,

Andres Freund