Re: Minor ON CONFLICT related fixes

On Tue, May 12, 2015 at 2:40 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> 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?

Evidently you're right. I'm not opposed to supporting ON CONFLICT
UPDATE with CREATE RULE, even if such rules are completely wonky. It
might be a useful way of testing the implementation.

>> 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.

The omissions you mention should be corrected on general principle, I think.

> 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.

Fair point.

> 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.

Okay.
--
Peter Geoghegan