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