Re: INSERT ... ON CONFLICT IGNORE (and UPDATE) 3.0

On 2015-04-22 15:23:16 -0700, Peter Geoghegan wrote:
> On Tue, Apr 21, 2015 at 7:57 AM, Andres Freund <andres@anarazel.de> wrote:
> > * Iff we're going to have the XLOG_HEAP_AFFIRM record, I'd rather have
> >   that guide the logical decoding code. Seems slightly cleaner.
> 
> I thought that you didn't think that would always work out? That in
> some possible scenario it could break?

I don't think there's a real problem. You obviously have to do it right
(i.e. only abort insertion if there's a insert/update/delete or commit).

Speaking of commits: Without having rechecked: I think you're missing
cleanup of th pending insertion on commit.

> > * Gram.y needs a bit more discussion:
> >   * Can anybody see a problem with changing the precedence of DISTINCT &
> >     ON to nonassoc? Right now I don't see a problem given both are
> >     reserved keywords already.
> 
> Why did you push code that solved this in a roundabout way, but
> without actually reverting my original nonassoc changes (which would
> now not result in shift/reduce conflicts?).

I pushed the changes to a separate repo so you could polish them ;)

> What should we do about that?

I'm prety sure we should not do the %nonassoc stuff.

> I don't like that you seem to have regressed
> diagnostic output [1].

Meh^5. This is the same type of errors we get in literally hundreds of
other syntax errors. And in contrast to the old error it'll actually
have a correct error possition.

> Surely it's simpler to use the nonassoc approach?

I think it's much harder to forsee all consequences of that.

> > * SPEC_IGNORE,  /* INSERT of "ON CONFLICT IGNORE" */ looks like
> >   a wrongly copied comment.
> 
> Not sure what you mean here. Please clarify.

I'm not sure either ;)

> > * I wonder if we now couldn't avoid changing heap_delete's API - we can
> >   always super delete if we find a speculative insertion now. It'd be
> >   nice not to break out of core callers if not necessary.
> 
> Maybe, but if there is a useful way to break out only a small subset
> of heap_delete(), I'm not seeing it.

I think you misread my statement: I'm saying we don't need the new
argument anymore, even if we still do the super-deletion in
heap_delete(). Now that the speculative insertion will not be visible
(as in seen on a tuple they could delete) to other backends we can just
do the super deletion if we see that the tuple is a promise one.

> > * breinbaas on IRC just mentioned that it'd be nice to have upsert as a
> >   link in the insert. Given that that's the pervasive term that doesn't
> >   seem absurd.
> 
> Not sure what you mean. Where would the link appear?

The index, i.e. it'd just be another indexterm. It seems good to give
people a link.

> * We need to figure out the tuple lock strength details. I think this
> is doable, but it is the greatest challenge to committing ON CONFLICT
> UPDATE at this point. Andres feels that we should require no greater
> lock strength than an equivalent UPDATE. I suggest we get to this
> after committing the IGNORE variant. We probably need to discuss this
> some more.

I want to see a clear way forward before we commit parts. It doesn't
have to be completely iron-clad, but the way forward should be pretty
clear. What's the problem you're seeing right now making this work? A
couple days on jabber you seemed to see a way doing this?

The reason I think this has to use the appropriate lock level is that
it'll otherwise re-introduce the deadlocks that fk locks resolved. Given
how much pain we endured to get fk locks, that seems like a bad deal.

Greetings,

Andres Freund