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

Hi,

On 2013-12-24 13:18:36 -0800, Peter Geoghegan wrote:
> On Tue, Dec 24, 2013 at 4:09 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > I don't really see the lack of review as being crucial at this point. At
> > least I have quite some doubts about the approach you've chosen and I
> > have voiced it - so have others.
> 
> Apparently you haven't been keeping up with this thread. The approach
> that Heikki outlined with his POC patch was demonstrated to deadlock
> in an unprincipled manner - it took me a relatively long time to
> figure this out because I didn't try a simple enough test-case.

So? I still have the fear that you approach will end up being way too
complicated and full of layering violations. I didn't say it's a no-go
(not that I have veto powers, even if I'd consider it one).

And yes, I still think that promise tuples might be a better solution
regardless of the issues you mentioned, but you know what? That doesn't
matter. Me thinking it's the better approach is primarily based on gut
feeling, and I clearly haven't voiced clear enough reasons to convince
you. So you going with your own, possibly more substantiated, gut
feeling is perfectly alright. Unless I go ahead and write a POC of my
own at least ;)

> In hindsight I should have known better than to think that people
> would be willing to defer discussion of a more acceptable value
> locking implementation to consider the interactions between the
> different subsystems, which I felt were critical and warranted
> up-front discussion, a belief which has now been borne out.
> Lesson learned. It's a pity that that's the way things are, because that
> discussion could have been really useful, and saved us all some time.

Whoa? What? Not convincing everyone is far from it being a useless
discussion. Such an attitude sure is not the way to go to elicit more
feedback.
And it clearly gave you the feedback that most people regard holding
buffer locks across other nontrivial operations, in a potentially
unbounded number, as a fundamental problem.

> > I don't think there's too much reviewers can do before you've provided a
> > POC implementation of real value locking.
> 
> I don't see what is functionally insufficient about the value locking
> that you've already seen.

I still think it's fundamentally unacceptable to hold buffer locks
across any additional complex operations. So yes, I think the current
state is fundamentally insufficient.
Note that the case of the existing uniqueness checking already is bad,
but it at least will never run any user defined code in that context,
just HeapTupleSatisfies* and HOT code. So I don't think arguments of the
"we're already doing it in uniqueness checking" ilk have much merit.

> If you're  still of the opinion that it is necessary to hold value locks of some
> form on earlier unique indexes, as you wait maybe for hours on some
> conflicting xid, then I still disagree with you for reasons recently
> re-stated [1].

I guess you're referring to:

On 2013-12-23 14:59:31 -0800, Peter Geoghegan wrote:
> Holding value locks for more than an instant doesn't make sense. The
> reason is simple: when upserting, we're tacitly only really looking
> for violations on one particular unique index. We just lock them all
> at once because the implementation doesn't know *which* unique index.
> So in actuality, it's really no different from existing
> potential-violation handling for unique indexes, except we have to do
> some extra work in addition to the usual restart from scratch stuff
> (iff we have multiple unique indexes).

I think the point here really is that that you assume that we're always
only looking for conflicts with one unique index. If that's all we want
to support - sure, only the keys in that index need to be locked.
I don't think that's necessarily a given, especially when you just want
to look at the conflict in detail, without using a subtransaction.

> You never stated a reason why you thought it was
> necessary. If you have one now, please share it. Note that I release
> all value locks before row locking too, which is necessary because to
> do any less will cause unprincipled deadlocking, as we've seen.

I can't sensibly comment upon that right now, I'd need to read more code
to understand what you're doing there.

> Other than that, I have no idea what your continued objection to my
> design would be once the buffer level exclusive locks are replaced
> with page level heavyweight locks across complex (though brief)
> operations

Well, you haven't delivered that part yet, that's pretty much my point,
no?
I don't think you can easily do this by just additionally taking a new
kind of heavyweight locks in the new codepaths - that will still allow
deadlocks with the old codepaths taking only lwlocks. So this is a
nontrivial sub-project which very well might influence whether the
approach is deemed acceptable or not.

> (I guess you might not like the visibility stuff or the
> syntax, but that isn't what you're talking about here).

I don't particularly care about that for now. I think we can find common
ground, even if it will take some further emails. It probably isn't
what's in there right now, but I don't think you'e intended it as such.

I don't think the visibility modifications are a good thing (or correct)
as is, but I don't think they are neccessary for your approach to make
sense.

> I've been very consistent even in the face of strong criticism. What I
> have now is essentially the same design as back in early September.

Uh. And why's that necessarily a good thing?


Minor details I noticed in passing:
* Your tqual.c bit isn't correct, you're forgetting multixacts.
* You several mention "core" in comments as if this wouldn't be part of it, that seems confusing.
* Doesn't ExecInsert() essentially busy-loop if there's a concurrent non-committed insert?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services