Re: Minor ON CONFLICT related fixes

On 2015-05-11 20:16:00 -0700, Peter Geoghegan wrote:
> On Mon, May 11, 2015 at 7:34 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > You should try to understand why it's failing. Just prohibiting the
> > rules, without understanding what's actually going on, could very well
> > hide a real bug.
>
> It's not as if I have no idea. ReplaceVarsFromTargetList() is probably
> quite confused by all this, because the passed nomatch_varno argument
> is often rt_index -- but what about EXCLUDED.*? adjustJoinTreeList()
> does not know anything about EXCLUDED.* either. I see little practical
> reason to make the rewriter do any better.

I don't think any of these are actually influenced by upsert?

> When I debugged the problem of the optimizer raising that "target
> lists" error with a rule with an action with EXCLUDED.* (within
> setrefs.c's fix_join_expr_mutator()), it looked like an off-by-one
> issue here:
>
> /* If it's for acceptable_rel, adjust and return it */
> if (var->varno == context->acceptable_rel)
> {
> var = copyVar(var);
> var->varno += context->rtoffset;
> if (var->varnoold > 0)
> var->varnoold += context->rtoffset;
> return (Node *) var;
> }

That's just a straight up bug. expression_tree_walker (called via
ChangeVarNodes) did not know about exclRelTlist, leading to
fix_join_expr() not matching the excluded vars to the excluded
relation/tlist.

I'm not immediately seing how that could bit us otherwise today, but
it'd definitely otherwise be a trap. That's why I think it's unwise to
ignore problems before having fully debugged them.

Additionally OffsetVarNodes() did not adjust exclRelIndex, which
"breaks" explain insofar it's not going to display the 'excluded.'
anymore. I don't think it could have other consequences.

Greetings,

Andres Freund