Re: Minor ON CONFLICT related fixes - Mailing list pgsql-hackers

From Peter Geoghegan
Subject Re: Minor ON CONFLICT related fixes
Date
Msg-id CAM3SWZSJ_pfUjB0vnu2pTr+95d8=uwz-HQ+5_RsXaccB39-MGw@mail.gmail.com
Whole thread Raw
In response to Re: Minor ON CONFLICT related fixes  (Andres Freund <andres@anarazel.de>)
List pgsql-hackers
On Tue, May 12, 2015 at 2:40 PM, Andres Freund <andres@anarazel.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



pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: Minor ON CONFLICT related fixes
Next
From: Andres Freund
Date:
Subject: Re: Minor ON CONFLICT related fixes