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

On Fri, Sep 26, 2014 at 5:24 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> 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.

Right. It was a rough prototype built to prove a point. It also served
to show what I was talking about as regards deadlocks (and how the
locks could problematically persist in other ways), which I was
previously unable to effectively explain to Andres. So it was a very
useful exercise, and I wish we did that kind of thing more frequently.
But at the same time, I don't want to hold you to that prototype, or
misrepresent that prototype as showing your final position on any
technical issue. So please correct me if I do that. I've tried to be
careful about that.

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

My concern with the xmin stuff is not that it's ugly; it's that it's
potentially dangerous. It isn't at all easy to reason about where bugs
might appear - lots of things could interact with it in unpredictable
ways. I think we'd have to audit a lot of code, all over the place,
just to make sure nowhere had an assumption broken. This is a big
issue. You are asking me to find a way to save a design that I don't
particularly believe in. That might change, but right now I'm afraid
that that's the reality. Whereas, my design is entirely contained in
the file nbtinsert.c.

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

I mean that "value locking" is an abstraction that lives in the head
of amcanunique AMs. That kind of encapsulation has considerable value
in reducing the risk of bugs. If what I've done has bugs, there isn't
that many places that could expose interactions with other complicated
code. There are fewer moving parts. It's a generalization of the
existing mechanism for unique index enforcement. Plus, database
systems have used heavyweight index locks for this kind of thing since
the 1970s. That's how this works everywhere else (SQL server certainly
does this for MERGE [1], and only grabs the page-level lock for a
second at lower isolation levels, as in my implementation). I think
that that ought to count for something.

I will be frank. Everyone knows that the nbtree locking parts of this
are never going to be committed over your objections. It cannot
happen. And yet, I persist in proposing that we go that way. I may be
stubborn, but I am not so stubborn that I'd jeopardize all the work
I've put into this to save one aspect of it that no one really cares
about anyway (even I only care about meeting my goals for user visible
behavior [2]). I may actually come up with a better way to make what
you outline work; then again, I may not. I have no idea, to be honest.
It's pretty clear that I'm going to have a hard time getting your
basic approach to value locking accepted without rethinking it a lot,
though. Can you really say that you won't have serious misgivings
about something like the "tuple->xmin = InvalidTransactionId"
swapping, if I actually formally propose it? That's very invasive to a
lot of places. And right now, I have no idea how we could do better.

I really only want to get to where we have a design that's acceptable.
In all sincerity, I may yet be convinced to go your way. It's possible
that I've failed to fully understand your concerns. Is it really just
about making INSERT ... ON CONFLICT IGNORE work with exclusion
constraints (UPDATE clearly makes little sense)?

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

I'll think about it. I don't want to do that until I see a way to make
your approach to value locking work in a way that someone is actually
going to be comfortable committing. I am looking for one.

By the way, IMO stress testing has a very useful role to play in the
development of this feature. I've been doing things like trying to
flush out races by running long stress tests with random delays
artificially added at key points. I would like to make that part of
the testing strategy public and transparent.

[1] http://weblogs.sqlteam.com/mladenp/archive/2007/08/03/60277.aspx
[2] Slide 8, "Goals for UPSERT in Postgres":
http://www.pgcon.org/2014/schedule/attachments/327_upsert_weird.pdf
--
Peter Geoghegan