On 2017-03-23 21:58:03 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-03-23 21:26:19 -0400, Tom Lane wrote:
> >> Hmm, I see ... but that only works in the cases where the caller of
> >> ExecBuildProjectionInfo supplied a source slot, and a lot of 'em
> >> don't.
>
> > Right, the old and new code comment on that:
>
> > * inputDesc can be NULL, but if it is not, we check to see whether simple
> > * Vars in the tlist match the descriptor. It is important to provide
> > * inputDesc for relation-scan plan nodes, as a cross check that the relation
> > * hasn't been changed since the plan was made. At higher levels of a plan,
> > * there is no need to recheck.
>
> Ah, I'd forgotten the assumption that we only need to check this at scan
> level.
>
> >> I'd already put in the infrastructure to add ASSIGN_FOO_VAR_FIRST
> >> step types. I could take it back out, but I wonder if it wouldn't be
> >> smarter to keep it and remove the restriction in ExecBuildProjectionInfo.
> >> Or maybe we could have ExecBuildProjectionInfo emit either
> >> ASSIGN_FOO_VAR_FIRST or ASSIGN_FOO_VAR depending on whether it can prove
> >> the reference safe.
>
> > I think it's probably ok to just leave the check in, and remove those
> > comments, and simplify the isSimpleVar stuff to only check if
> > IsA(tle->expr, Var) && ((Var *) tle->expr)->varattno > 0)
>
> Not sure. It's a pretty fair amount of duplicative code, once you finish
> dealing with all the ExecJustFoo functions in addition to the main code
> paths. At this point I'm inclined to take it back out and improve the
> comments around ExecBuildProjectionInfo.
I'm ok with both.
- Andres
