Re: INSERT...ON DUPLICATE KEY LOCK FOR UPDATE - Mailing list pgsql-hackers

On 11/26/13 01:59, Peter Geoghegan wrote:
> On Sat, Nov 23, 2013 at 11:52 PM, Peter Geoghegan <pg@heroku.com> wrote:
>> I have some concerns about what you've done that may limit my
>> immediate ability to judge performance, and the relative merits of
>> both approaches generally. Now, I know you just wanted to sketch
>> something out, and that's fine, but I'm only sharing my thoughts. I am
>> particularly worried about the worst case (for either approach),
>> particularly with more than 1 unique index. I am also worried about
>> livelock hazards (again, in particular with more than 1 index) - I am
>> not asserting that they exist in your patch, but they are definitely
>> more difficult to reason about. Value locking works because once a
>> page lock is acquired, all unique indexes are inserted into. Could you
>> have two upserters livelock each other with two unique indexes with
>> 1:1 correlated values in practice (i.e. 2 unique indexes that might
>> almost work as 1 composite index)? That is a reasonable usage of
>> upsert, I think.
>
> So I had it backwards: In fact, it isn't possible to get your patch to
> deadlock when it should - it livelocks instead (where with my patch,
> as far as I can tell, we predictably and correctly have detected
> deadlocks). I see an infinite succession of "insertion conflicted
> after pre-check" DEBUG1 elog messages, and no progress, which is an
> obvious indication of livelock. My test does involve 2 unique indexes
> - that's generally the hard case to get right. Dozens of backends are
> tied-up in livelock.
>
> Test case for this is attached.

Great, thanks! I forgot to reset the "conflicted" variable when looping
to retry, so that once it got into the "insertion conflicted after
pre-check" situation, it never got out of it.

After fixing that bug, I'm getting a correctly-detected deadlock every
now and then with that test case.

> I'm also seeing this:
>
> Client 45 aborted in state 2: ERROR:  attempted to lock invisible tuple
> Client 55 aborted in state 2: ERROR:  attempted to lock invisible tuple
> Client 41 aborted in state 2: ERROR:  attempted to lock invisible tuple

Hmm. That's because the trick I used to kill the just-inserted tuple
confuses a concurrent heap_lock_tuple call. It doesn't expect the tuple
it's locking to become invisible. Actually, doesn't your patch have the
same bug? If you're about to lock a tuple in ON DUPLICATE KEY LOCK FOR
UPDATE, and the transaction that inserted the duplicate row aborts just
before the heap_lock_tuple() call, I think you'd also see that error.

> To me this seems like a problem with the (potential) total lack of
> locking that your approach takes (inserting btree unique index tuples
> as in your patch is a form of value locking...sort of...it's a little
> hard to reason about as presented). Do you think this might be an
> inherent problem, or can you suggest a way to make your approach still
> work?

Just garden-variety bugs :-). Attached patch fixes both issues.

> So I probably should have previously listed as a requirement for our design:
>
> * Doesn't just work with one unique index. Naming a unique index
> directly in DML, or assuming that the PK is intended seems quite weak
> to me.
>
> This is something I discussed plenty with Robert, and I guess I just
> forgot to repeat myself when asked.

Totally agreed on that.

- Heikki