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

Andres Freund <andres@anarazel.de> writes:
> Afaict the mtstate->ps.plan->targetlist assignment, and the ExecTypeFromTL(),
> ExecAssignResultTypeFromTL() before that, are completely superfluous.  Am I
> missing something?

I think you are right.  The two tlists are completely identical in
most cases, because of this bit in setrefs.c:

                    /*
                     * Set up the visible plan targetlist as being the same as
                     * the first RETURNING list. This is for the use of
                     * EXPLAIN; the executor won't pay any attention to the
                     * targetlist.  We postpone this step until here so that
                     * we don't have to do set_returning_clause_references()
                     * twice on identical targetlists.
                     */
                    splan->plan.targetlist = copyObject(linitial(newRL));

I added a quick check

+       if (!equal(mtstate->ps.plan->targetlist,
+                  linitial(returningLists)))
+           elog(WARNING, "not matched targetlist");

to verify that.  There's one regression case in which it complains,
and that's one where we pruned the first result relation so that
linitial(returningLists) is now the second result rel's RETURNING
list.  But as you say, that should still produce the same tupdesc.
I think we can just delete this assignment (and fix these comments).

> Wonder if the targetlist assignment is superfluous made me wonder if we would
> detect mismatches - and afaict we largely wouldn't. There's basically no
> verification in ExecBuildProjectionInfo().  And indeed, adding something very
> basic shows:

Hmm, seems like an independent issue.

            regards, tom lane