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

On Mon, Dec 30, 2013 at 8:19 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Maybe you should describe what you mean with "unprincipled". Sure, the
> current patch deadlocks, but I don't see anything fundamental,
> unresolvable there. So I don't understand what the word unprincipled
> means in that sentence..

Maybe it is resolvable, and maybe it's worth resolving - I never said
that it wasn't, I just said that I doubt the latter. By unprincipled
deadlocking, I mean deadlocking that cannot be reasonably prevented by
a user. Currently, I think that never deadlocking is a reasonable
aspiration for all applications. It's never really necessary. When it
occurs, we can advise users to do simple analysis and application
refactoring to prevent it. With unprincipled deadlocking, we can give
no such advice. The only advice we can give is to stop doing so much
upserting, which is a big departure from how things are today. AFAICT,
no one disagrees with my view that this is bad, and probably
unacceptable.

> Uh. But that was said in the context of *your* approach being
> flawed. Because it - at that time, I didn't look at the newest version
> yet - extended the concept of holding btree page locks across external
> operation to far much more code, even including user defined code!. And
> you argued that that isn't a problem using _bt_check_unique() as an
> argument.

That's a distortion of my position at the time. I acknowledged from
the start that all buffer locking was problematic (e.g. [1]), and was
exploring alternative locking approaches and the merit of the design.
This is obviously the kind of project that needs to be worked at
through iterative prototyping. While arguing that deadlocking would
not occur, I lost sight of the bigger picture. But even if that wasn't
true, I don't know why you feel the need to go on and on about buffer
locking like this months later. Are you trying to be provocative? Can
you *please* stop?

Everyone knows that the btree heap access is a modularity violation.
Even the AM docs says that the heap access is "without a doubt ugly
and non-modular". So my original point remains, which is that
expanding that is obviously going to be controversial, and probably
legitimately so. I thought that your earlier marks on
_bt_check_unique() were a good example of this sentiment, but I hardly
need such an example.

> I don't really see why your patch is less of a modularity violation than
> Heikki's POC. It's just a different direction.

My approach does not regress modularity because it doesn't do anything
extra with the heap at all, and only btree insertion routines are
affected. Locking is totally localized to the btree insertion routines
- one .c file. At no other point does anything else have to care, and
it's obvious that this situation won't change in the future when we
decide to do something else cute with infomask bits or whatever.
That's a *huge* distinction.

[1] http://www.postgresql.org/message-id/CAM3SWZR2X4HJg7rjn0K4+hFdguCYX2prEP0Y3a7nccEjEowqqw@mail.gmail.com
--
Peter Geoghegan