Re: INSERT...ON DUPLICATE KEY LOCK FOR UPDATE

On Sat, Sep 14, 2013 at 3:15 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Well, the reality is that the promises idea hasn't been described in
>> remotely enough detail to compare it to what I have here. I've pointed
>> out plenty of problems with it.
>
> Even if you disagree, I still think that doesn't matter in the very
> least.

It matters if you care about getting this feature.

> You say:
>
>> I think that the details of how this approach compare to others are
>> totally pertinent. For me, that's the whole point - getting towards
>> something that will balance all of these concerns and be acceptable.
>
> Well, the two other people involved in the discussion so far have gone
> on the record saying that the presented approach is not acceptable to
> them. And you haven't started reacting to that.

Uh, yes I have. I'm not really sure what you could mean by that. What
am I refusing to address?

>> Yes, it's entirely possible that that could look quite different to
>> what I have here. I do not want to reduce all this to a question of
>> "is this one design acceptable or not?".
>
> But the way you're discussing it so far is exactly reducing it that way.

The fact that I was motivated to do things this way serves to
illustrate the problems generally.

> If you want the discussion to be about *how* can we implement it that
> the various concerns are addressed: fsck*ing great. I am with you there.

Isn't that what we were doing? There has been plenty of commentary on
alternative approaches.

> In the end, even though I have my usual strong opinions which is the
> best way, I don't care which algorithm gets pursued further. At least,
> if, and only if, it has a fighting chance of getting committed. Which
> this doesn't.

I don't think that any design that has been described to date doesn't
have serious problems. Causing excessive bloat, particularly in
indexes is a serious problem also.

>> The reason I ended up here is not because I didn't get the memo about
>> holding buffer locks across complex operations being a bad thing. At
>> least grant me that. I'm here because in all these years no one has
>> come up with a suggestion that doesn't have some very major downsides.
>> Like, even worse than this.
>
> I think you're massively, massively, massively overstating the dangers
> of bloat here. It's a known problem that's *NOT* getting worse by any of
> the other proposals if you compare it with the loop/lock/catch
> implementation of upsert that we have today as the only option.

Why would I compare it with that? That's terrible, and very few of our
users actually know about it anyway. Also, will an UPDATE followed by
an INSERT really bloat all that much anyway?

> And we
> *DO* have infrastructure to deal with bloat, even if could use some
> improvement. We *don't* have infrastructure with deadlocks on
> lwlocks. And we're not goint to get that infrastructure, because it
> would even further remove the "lw" part of lwlocks.

Everything I said so far is predicated on LWLocks not deadlocking
here, so I'm not really sure why you'd say that. If I can't find a way
to prevent deadlock, then clearly the approach is doomed.

> It's that it interleaves an already complex but local locking scheme
> that required several years to become correct with another that is just
> the same. That's an utterly horrid idea.

You're missing my point, which is that it may be possible, with
relatively modest effort, to analyze things to ensure that deadlock is
impossible - regardless of the complexities of the two systems -
because they're reasonably well encapsulated. See below, under "I'll
say it again".

Now, I can certainly understand why you wouldn't be willing to accept
that at face value. The idea isn't absurd, though. You could think of
the heap_insert() call as being under the control of the btree code
(just as, say, heap_hot_search() is), even though the code isn't at
all structured that way, and that's awkward. I'm actually slightly
tempted to structure it that way.

>> Is that really so different to holding an exclusive
>> lock on a btree buffer while holding a shared lock on a heap buffer?
>> Because that's what _bt_check_unique() does today.
>
> Yes, it it is different. But, in my opinion, bt_check_unique() doing so
> is a bug that needs fixing. Not something that we want to extend.

Well, I think you know that that's never going to happen. There are
all kinds of reasons why it works that way that cannot be disavowed.
My definition of a bug includes a user being affected.

>> > At this point I am a bit confused why you are asking for review.
>>
>> I am asking for us, collectively, through consensus, to resolve the
>> basic approach to doing this. That was something I stated right up
>> front, pointing out details of where the discussion had gone in the
>> past. That was my explicit goal. There has been plenty of discussing
>> on this down through the years, but nothing ever came from it.
>
> At the moment ISTM you're not conceding on *ANY* points. That's not very
> often the way to find concensus.

Really? I've conceded plenty of points. Just now I conceded a point to
Robert about insertion being blocked for inserters that want to insert
a value that isn't already locked/existing, and he didn't even raise
that in the first place. Most prominently, I've conceded that it is
entirely questionable that I hold the buffer locks for longer - before
you even responded to my original patch! I've said it many many times
many many ways. It should be heavily scrutinized. But you both seem to
be making general points along those lines, without reference to what
I've actually done. Those general points could almost to the same
extent apply to _bt_check_unique() today, which is why I have a hard
time accepting them at face value. To say that what that function does
is "a bug" is just not credible, because it's been around in
essentially the same form since at least a time when you and I were in
primary school. I'll remind you that you haven't been able to
demonstrate deadlock in a way that invalidates my approach. While of
course that's not how this is supposed to work, I've been too busy
defending myself here to get down to the business of carefully
analysing the relatively modest interactions between btree and heap
that could conceivably introduce a deadlock. Yes, the burden to prove
this can't deadlock is mine, but I thought I'd provide you with the
opportunity to prove that it can.

I'll say it again: For a deadlock, there needs to be a mutual
dependency. Provided the locking phase doesn't acquire any locks other
than buffer locks, and during the interaction with the heap btree
inserters (or the locking phase) cannot acquire heap locks in a way
that conflicts with other upserters, we will be fine. It doesn't
necessarily matter how complex each system individually is, because
the two meet in such a limited area (well, two areas now, I suppose),
and they only meet in one direction - there is no reciprocation where
the heap code locks or otherwise interacts with index buffers. When
the heap insertion is performed, all index value locks are already
acquired. The locking phase cannot block itself because of the
ordering of locking, but also because the locks on the heap that it
takes are only shared locks.

Now, this analysis is somewhat complex, and underdeveloped. But as
Robert said, there are plenty of things about locking in Postgres that
are complex and subtle. He also said that it doesn't matter if I can
prove that it won't deadlock, but I'd like a second opinion on that,
since my proof might actually be, if not simple, short, and therefore
may not represent an ongoing burden in the way Robert seemed to think
it would.

> That argument doesn't make sense to me. You're inserting a unique
> value. It completely makes sense that you can only insert one of
> them.

> Even if it had an advantage, not blocking *for the single unique key alone*
> opens you to issues of livelocks where several backends retry because of
> each other indefinitely.

See my remarks to Robert.

> Whether postgres locks down in a way that can only resolved by kill -9
> or whether it aborts a transaction are, like, a couple of magnitude of a
> difference.

Not really. I can see the advantage of having the deadlock be
detectable from a defensive-coding standpoint. But index locking
ordering inconsistencies, and the deadlocks they may cause are not
acceptable generally.

--
Peter Geoghegan