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

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