Andres Freund <and...@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
