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

On Fri, Dec 27, 2013 at 12:57 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> 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.

I was attempting to make a point about the controversy this generated
in September. Which is: we were talking past each other. It was an
unfortunate, unproductive use of our time - there was some useful
discussion, but in general far more heat than light was generated. I
don't want to play the blame game. I want to avoid that situation in
the future, since it's obvious to me that it was totally avoidable.
Okay?

> 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.

Well, you're not totally on your own for something like that with this
feature. You can project the conflicter's tid, and possibly do a more
sophisticated recovery, like inspecting the locked row and iterating.
That's probably not at all ideal, but then I can't even imagine what
the best interface for what you describe here looks like. If there are
multiple conflicts, do you delete or update some or all of them? How
do you express that concept from a DML statement? Maybe you could
project the conflict rows (with perhaps more than 1 for each row
proposed for insertion) and not the rejected, but it's hard to imagine
making that intuitive or succinct (conflicting rows could be projected
twice or more for separate conflicts, etc). Maybe what I have here is
in practical terms actually a pretty decent approximation of what you
want.

It seems undesirable to give other use-cases baggage around locking
values for an indefinite period, just to make this work for MM
replication, especially since it isn't clear that it actually could be
used effectively by a MM replication solution given the syntax, or any
conceivable alternative syntax or variant.

Could SQL MERGE do this for you? Offhand I don't think that it could.
In fact, I think it would be much less useful than what I've proposed
for this use-case. Its "WHEN NOT MATCHED THEN" clause doesn't let you
introspect details of what matched and did not match. Furthermore,
though I haven't verified this, off-hand I suspect other systems are
fussy about what you want to merge on. All examples of MERGE use I've
found after a quick Google search shows merging on a simple equi-join
criteria.

>> 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.

I spent several weeks earnestly thrashing out details of Heikki's
design. I am open to any alternative design that meets the criteria I
outlined to Heikki, with which Heikki was in full agreement. One of
those criterions was that unprincipled deadlocking, that would never
occur with equivalent update statements should not occur.
Unfortunately, Heikki's POC patch did not meet that standard. I have
limited enthusiasm for making it or a similar scheme meet that
standard by further burdening the btree AM with additional knowledge
of the heap or row locking. Since in the past you've expressed general
concerns about the modularity violation within the btree AM today, I
assume that you aren't too enthusiastic about that kind of expansion
either.

> 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

That's right. Just FOR UPDATE locks.

> but using
> subtransactions you still can get into such a scenario, even involving
> FOR UPDATE locks.

Sigh.

-- 
Peter Geoghegan