On Sat, Sep 19, 2015 at 5:11 PM, Andres Freund <andres@anarazel.de> wrote:
> Peter's patch upthread fixes this by pulling expressions from
> onConflictSet/Where into the targetlist. I disliked this - much less
> than initially - a bit because that seems a bit crufty given that we're
> not actually getting data from a child node. This is different to
> RETURNING where the targetlist massaging is actually important to get
> the data up the tree.
Maybe the massaging is somewhat justified by the fact that it's just
as good a place as any to reject system columns, and that needs to
happen somewhere. I know that you suggested that this be done during
parse analysis, but not sure how attached you are to that. Might also
be a good choke point for detecting when unexpected vars/expressions
appear in the targetlist due to unforeseen circumstances/bugs. I
actually cover a couple of "can't happen" cases at the same time,
IIRC.
Continuing to follow RETURNING may have some value, even if the
analogy is a bit more forced here.
> An actually trivial, although not all that pretty, fix is to simply
> accept wholerow references in fix_join_expr_mutator(), even if not in
> the targetlist. As far as I can see the problem right now really can
> only be hit for whole row references.
I am concerned about the risk of adding bugs to unrelated code paths
that this could create. I must admit that this concern is mostly
driven by paranoia, and not a seasoned appreciation of problems that
could arise from ordinary post-processing of join expressions.
> A variant of the second approach is to have a fix_onconflict_expr()
> mutator that has such special handler.
I suppose you could add a fix_join_expr_context field that had
fix_join_expr_mutator() avoid the special handler for post-processing
of join expressions. That might be a bit ugly too, but would involve
less code duplication.
> Any opinions on either approach?
I think that I favor my original solution, although only by a tiny
margin. I will avoid offering either a -1 or a +1 to any proposal
here, although they all sound basically reasonable to me. A more
complete targetlist representation would have been something that I'd
probably vote against, since it seems complex and invasive, but that
doesn't matter now. In short, I defer to others here.
--
Peter Geoghegan