Re: INSERT ... ON CONFLICT {UPDATE | IGNORE}

On 09/26/2014 12:13 AM, Peter Geoghegan wrote:
> On Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> C. Internal weirdness
>> Submit C based upon earlier agreed B, submit to Dec 15 CF, major patch
>> deadline, so we can fine tune for last CF.
>> Then Heikki rewrites half your patch in a better way, you thank him
>> and then we commit. All done.
>
> I don't have a problem with Heikki or anyone else rewriting the value
> locking part of the patch, provided it meets my requirements for such
> a mechanism. Since Heikki already agreed that that standard should be
> imposed, he'd hardly take issue with it now.
>
> However, the fact is that once you actually make something like
> promise tuples meet that standard, at the very least it becomes a lot
> messier than you'd think. Heikki's final prototype "super deleted"
> tuples by setting their xmin to InvalidTransactionId. We weren't sure
> that that doesn't break some random other heapam code. Consider this,
> for example:
>
> https://github.com/postgres/postgres/blob/REL9_4_STABLE/src/backend/executor/execMain.c#L1961
>
> So that looks safe in the face of setting xmin to InvalidTransactionId
> in the way the later prototype patch did if you think about it for a
> while, but there are other places where that is less clear. In short,
> it becomes something that we have to worry about for ever, because
> "xmin cannot change without the tuple in the slot changing" is clearly
> an invariant for certain purposes. It might accidentally fail to fail
> right now, but I'm not comfortable with it.

Just to be clear: I wrote the initial patch to demonstrate what I had in
mind, because I was not able to explain it well enough otherwise. You
pointed out issues with it, which I then fixed. You then pointed out
more issues, which I then fixed again.

My patch version was a proof of concept, to demonstrate that it can be
done. What I'd like you to do now, as the patch author, is to take the
promise tuple approach and clean it up. If the xmin stuff is ugly,
figure out some other way to do it.

> Now, I might be convinced that that's actually the way to go. I have
> an open mind. But that will take discussion. I like that page
> hwlocking is something that many systems do (even including Oracle, I
> believe). Making big changes to nbtree is always something that
> deserves to be met with skepticism, but it is nice to have an
> implementation that lives in the head of AM.

I don't know what you mean by "in the head of AM", but IMO it would be
far better if we can implement this outside the index AMs. Then it will
work with any index AM.

BTW, in the discussions, you pointed out that exclusion constraints
currently behave differently from a unique index, when two backends
insert a tuple at the same time. With a unique index, one of them will
fail, but one is always guaranteed to succeed. With an exclusion
constraint, they can both fail if you're unlucky. I think the promise
tuples would allow us to fix that, too, while we're at it. In fact, you
might consider tackling that first, and build the new INSERT ON CONFLICT
syntax on top of that. Basically, an INSERT to a table with an exclusion
constraint would be the same as "INSERT ON CONFLICT throw an error".
That would be a useful way to split this patch into two parts.

- Heikki