Re: INSERT...ON DUPLICATE KEY LOCK FOR UPDATE
От | Andres Freund |
---|---|
Тема | Re: INSERT...ON DUPLICATE KEY LOCK FOR UPDATE |
Дата | |
Msg-id | 20131227085727.GB19290@alap2.anarazel.de обсуждение исходный текст |
Ответ на | Re: INSERT...ON DUPLICATE KEY LOCK FOR UPDATE (Peter Geoghegan <pg@heroku.com>) |
Ответы |
Re: INSERT...ON DUPLICATE KEY LOCK FOR UPDATE
(Peter Geoghegan <pg@heroku.com>)
Re: INSERT...ON DUPLICATE KEY LOCK FOR UPDATE (Peter Geoghegan <pg@heroku.com>) |
Список | pgsql-hackers |
Hi, On 2013-12-25 15:27:36 -0800, Peter Geoghegan wrote: > Uh, I knew that it was a problem all along. While I explored ways of > ameliorating the problem, I specifically stated that we should discuss > the subsystems interactions/design, which you were far too quick to > dismiss. Aha? > The overall design is far more pertinent than one specific > mechanism. While I certainly welcome your participation, if you want > to be an effective reviewer I suggest examining your own attitude. > Everyone wants this feature. You know what. I don't particularly feel the need to be a reviewer of this patch. I comment because there didn't seem enough comments on some parts and because I see some things as problematic. If you don't want those comments, ok. No problem. > >> 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. > > Why would I not assume that? It's perfectly obvious from the syntax > that you can't do much if you don't know ahead of time where the > conflict might be. Because it's a damn useful feature to have. As I said above: > if that's all we want to support - sure, only the keys in that index > need to be locked. I don't think the current syntax the feature implements can be used as the sole argument what the feature should be able to support. If you think from the angle of a async MM replication solution replicating a table with multiple unique keys, not having to specify a single index we to expect conflicts from, is surely helpful. > >> 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. > > You could have looked at it back in September, if only you'd given > these interactions the up-front consideration that they warranted. > Nothing has changed there at all. Holy fuck. Peter. Believe it or not, I don't remember all code, comments & design that I've read at some point. And that sometimes means that I need to re-read code to judge some things. That I don't have time to fully do so on the 24th doesn't strike me as particularly suprising. > > 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 have already written the code, and am in the process of cleaning it > up and gaining confidence that I haven't missed something. It's not > trivial, and there are some subtleties, but I think that your level of > skepticism around the difficulty of doing this is excessive. > Naturally, non-speculative insertion does have to care about the > heavyweight locks sometimes, but only when a page-level flag is found > to be set. Cool then. > >> 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? > > It isn't necessarily, but you've taken my comments out of context. It's demonstrative of the reaction to a good part of the doubts expressed. > Can we focus on the design, and how things fit together, > please? I don't understand you here. You want people to discuss the high level design but then criticize us for discussion the high level design when it involves *possibly* doing things differently. Evaluating approaches *is* focusing on the design. And saying that a basic constituent part doesn't work - like using the buffer locking for value locking, which you loudly doubted for some time - *is* design critizism. The pointed out weakness very well might be non-existant because of a misunderstanding, or relatively easily fixable. > > Minor details I noticed in passing: > > * Your tqual.c bit isn't correct, you're forgetting multixacts. > > I knew that was broken, but I don't grok the tuple locking code. > Perhaps you can suggest a correct formulation. I don't think there's nice high-level infrastructure to do what you need here yet. You probably need a variant of MultiXactIdIsRunning() like MultiXactIdAreMember() that checks whether any of our xids is participating. Which you then check when xmax is a multi. Unfortunately I am afraid that it won't be ok to check HEAP_XMAX_IS_LOCKED_ONLY xmaxes only - it might have been a no-key update + some concurrent key-share lock where the updater aborted. Now, I think you only acquire FOR UPDATE locks so far, but using subtransactions you still can get into such a scenario, even involving FOR UPDATE locks. > > * You several mention "core" in comments as if this wouldn't be part of > > it, that seems confusing. > > Well, the executor is naive of the underlying AM, even if it is btree. > What terminology do you suggest that captures that? I don't have a particularly nice suggestion. "generic" maybe. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
В списке pgsql-hackers по дате отправления: