Thread: Minor ON CONFLICT related fixes

Attached patch fixes some issues with ON CONFLICT DO UPDATE/DO
NOTHING. There is a commit message which explains the changes at a
high level. The only real bug fix is around deparsing by ruleutils.c.

Note that the patch proposes to de-support CREATE RULE with an
alternative action involving ON CONFLICT DO UPDATE. Such a rule seems
particularly questionable to me, and I'm pretty sure it was understood
that only ON CONFLICT DO NOTHING should be supported as an action for
a rule (recall that INSERT statements with ON CONFLICT can, in
general, never target relations with rules). At least, I believe
Heikki said that.

Thoughts?
--
Peter Geoghegan

On Mon, May 11, 2015 at 6:53 PM, Peter Geoghegan <pg@heroku.com> wrote:
> Attached patch fixes some issues with ON CONFLICT DO UPDATE/DO
> NOTHING. There is a commit message which explains the changes at a
> high level.

I just realized that there is a silly bug here. Attached is a fix that
applies on top of the original.


--
Peter Geoghegan

HI,

Don't have time to go through this in depth.

On 2015-05-11 18:53:22 -0700, Peter Geoghegan wrote:
> Note that the patch proposes to de-support CREATE RULE with an
> alternative action involving ON CONFLICT DO UPDATE. Such a rule seems
> particularly questionable to me, and I'm pretty sure it was understood
> that only ON CONFLICT DO NOTHING should be supported as an action for
> a rule (recall that INSERT statements with ON CONFLICT can, in
> general, never target relations with rules). At least, I believe
> Heikki said that.

> Deparsing with an inference clause is now correctly supported.  However,
> user-defined rules with ON CONFLICT DO UPDATE are now formally
> disallowed/unsupported.  It seemed there would be significant complexity
> involved in making this work correctly with the EXCLUDED.*
> pseudo-relation, which was not deemed worthwhile.  Such a user-defined
> rule seems very questionable anyway.

Do I understand correctly that you cut this out because there
essentially was a ruleutils bug with with EXCLUDED? If so, I don't find
that acceptable. I'm pretty strictly convincded that independent of rule
support, we shouldn't add things to insert queries that get_query_def
can't deparse.

Greetings,

Andres Freund

On Mon, May 11, 2015 at 7:11 PM, Andres Freund <andres@anarazel.de> wrote:
> Do I understand correctly that you cut this out because there
> essentially was a ruleutils bug with with EXCLUDED? If so, I don't find
> that acceptable. I'm pretty strictly convincded that independent of rule
> support, we shouldn't add things to insert queries that get_query_def
> can't deparse.

No, that wasn't the reason -- deparsing itself works fine. That code
remains within ruleutils.c because I agree with this principle of
yours.

I tested the deparsing logic before making the case added fail as
unsupported. If you remove the new ereport() call that relates to
non-suppport of ON CONFLICT DO UPDATE as a rule action, then the
CREATE RULE still succeeds, and you can deparse the query just fine
(by quering pg_rules, typically). You just get an error within the
optimizer following rewriting of a query involving the application of
a rule that specifies an ON CONFLICT DO UPDATE alternative/DO INSTEAD
action. I found it would say: "variable not found in subplan target
lists".

I can't say that I explored the question very thoroughly, but it seems
bothersome to have more than one relation involved in a Query involved
in rewriting.

-- 
Peter Geoghegan

On Mon, May 11, 2015 at 7:22 PM, Peter Geoghegan <pg@heroku.com> wrote:
> You just get an error within the
> optimizer following rewriting of a query involving the application of
> a rule that specifies an ON CONFLICT DO UPDATE alternative/DO INSTEAD
> action. I found it would say: "variable not found in subplan target
> lists".

BTW, that error was only raised when the EXCLUDED.* pseudo-alias was
actually used, which is why our previous testing missed it.

-- 
Peter Geoghegan

On 2015-05-11 19:33:02 -0700, Peter Geoghegan wrote:
> On Mon, May 11, 2015 at 7:22 PM, Peter Geoghegan <pg@heroku.com> wrote:
> > You just get an error within the
> > optimizer following rewriting of a query involving the application of
> > a rule that specifies an ON CONFLICT DO UPDATE alternative/DO INSTEAD
> > action. I found it would say: "variable not found in subplan target
> > lists".
> 
> BTW, that error was only raised when the EXCLUDED.* pseudo-alias was
> actually used, which is why our previous testing missed it.

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.

Greetings,

Andres Freund

On Mon, May 11, 2015 at 7:34 PM, Andres Freund <andres@anarazel.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.

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;
 
}

-- 
Peter Geoghegan

On 2015-05-11 20:16:00 -0700, Peter Geoghegan wrote:
> On Mon, May 11, 2015 at 7:34 PM, Andres Freund <andres@anarazel.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

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

On 2015-05-12 23:40:47 +0200, Andres Freund wrote:
> 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've fixed these, and added a minimum amount of tests. Pleas echeck
whether more are needed.

Could you rebase and adjust your patch? I'd rather have the inference
structure refactoring separate.

Greetings,

Andres Freund

On Tue, May 12, 2015 at 3:16 PM, Andres Freund <andres@anarazel.de> wrote:
> Could you rebase and adjust your patch? I'd rather have the inference
> structure refactoring separate.

Sure.


-- 
Peter Geoghegan

On Tue, May 12, 2015 at 3:18 PM, Peter Geoghegan <pg@heroku.com> wrote:
> On Tue, May 12, 2015 at 3:16 PM, Andres Freund <andres@anarazel.de> wrote:
>> Could you rebase and adjust your patch? I'd rather have the inference
>> structure refactoring separate.
>
> Sure.

Rebased version of patch is attached.

--
Peter Geoghegan

On Tue, May 12, 2015 at 4:23 PM, Peter Geoghegan <pg@heroku.com> wrote:
> Rebased version of patch is attached.

FYI, I found an unrelated bug within ruleutils.c (looks like the
targetlist kludge in set_deparse_planstate() isn't sufficiently
general):

postgres=# explain insert into upsert as u values('Bat', 'Bar') on
conflict (key) do update set val = excluded.val where exists (select 1
from upsert ii where ii.key = excluded.key);
ERROR:  XX000: bogus varno: 65000
LOCATION:  get_variable, ruleutils.c:5916

Obviously Vars of varno INNER_VAR are not expected here -- the subplan
makes set_deparse_planstate() not prepare get_variable() in the same
way that it prepares similar though simpler cases (e.g. no
subquery/subplan).

I don't have any bright ideas about how to fix this offhand. I believe
in principle that we ought to be able to fish through parent planstate
within set_deparse_planstate(), just in case it is called under these
circumstances, but that's pretty ugly, and is usually going to be
unnecessary. Doesn't look like there is a good way to delay the work
till get_variable() can see what looks like this case, either.
-- 
Peter Geoghegan

On Tue, May 12, 2015 at 3:16 PM, Andres Freund <andres@anarazel.de> wrote:
> Could you rebase and adjust your patch? I'd rather have the inference
> structure refactoring separate.

I realized that I didn't split out the patch as requested.

Here is a cumulative version of what was previously posted.

Thanks
--
Peter Geoghegan

On Mon, May 18, 2015 at 1:49 PM, Peter Geoghegan <pg@heroku.com> wrote:
> Here is a cumulative version of what was previously posted.

BTW, this could result in more calls to get_opclass_family() and
get_opclass_input_type() than before. However, because we'll have
eliminated so many candidates (e.g. by seeing that they're not unique
indexes) by the time the call to infer_collation_opclass_match() comes
anyway, and because it's so rare to have to specify an opclass at all,
I thought this was fine.

-- 
Peter Geoghegan

On Sat, May 16, 2015 at 11:48 PM, Peter Geoghegan <pg@heroku.com> wrote:
> FYI, I found an unrelated bug within ruleutils.c (looks like the
> targetlist kludge in set_deparse_planstate() isn't sufficiently
> general):
>
> postgres=# explain insert into upsert as u values('Bat', 'Bar') on
> conflict (key) do update set val = excluded.val where exists (select 1
> from upsert ii where ii.key = excluded.key);
> ERROR:  XX000: bogus varno: 65000
> LOCATION:  get_variable, ruleutils.c:5916

You pointed out that the reason for this trivial bug on Jabber, but
here's the obvious fix, including an EXPLAIN regression test.

--
Peter Geoghegan

On Mon, May 18, 2015 at 2:09 PM, Peter Geoghegan <pg@heroku.com> wrote:
> You pointed out that the reason for this trivial bug on Jabber, but
> here's the obvious fix, including an EXPLAIN regression test.

Also, I attach a patch adding ruleutils.c deparsing support for INSERT
statement target aliases. This was previously overlooked.

--
Peter Geoghegan

On 2015-05-18 19:09:27 -0700, Peter Geoghegan wrote:
> On Mon, May 18, 2015 at 2:09 PM, Peter Geoghegan <pg@heroku.com> wrote:
> > You pointed out that the reason for this trivial bug on Jabber, but
> > here's the obvious fix, including an EXPLAIN regression test.
> 
> Also, I attach a patch adding ruleutils.c deparsing support for INSERT
> statement target aliases. This was previously overlooked.

Pushed.

On Tue, May 19, 2015 at 2:23 PM, Andres Freund <andres@anarazel.de> wrote:
> Pushed.

I eyeballed the commit, and realized that I made a trivial error. New
patch attached fixing that.

Sorry for not getting this fix completely right first time around.
Don't know how I missed it.
--
Peter Geoghegan

On 2015-05-19 22:50:54 -0700, Peter Geoghegan wrote:
> On Tue, May 19, 2015 at 2:23 PM, Andres Freund <andres@anarazel.de> wrote:
> > Pushed.
> 
> I eyeballed the commit, and realized that I made a trivial error. New
> patch attached fixing that.
> 
> Sorry for not getting this fix completely right first time around.
> Don't know how I missed it.

Pushed, with expanded tests.