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

Andres Freund <andres@anarazel.de> writes:
> 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.

D'oh ... I had seen that branch before, but missed fixing it.
Yeah, the targetlist will be NIL already, but it's still wrong.

>> 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.

> There are *some* cases where this changes the explain output, but but the new
> output is more correct, I think:
> ...
> 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.

I don't believe that we guarantee EXPLAIN output to be 100% valid SQL,
so I doubt there's a correctness argument here; certainly it'd not
affect pg_dump.  I'm curious though: what was the test case you were
looking at?

            regards, tom lane