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

On Fri, Dec 13, 2013 at 4:06 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I spent a little bit of time looking at btreelock_insert_on_dup. AFAICT
> it executes FormIndexDatum() for later indexes while holding assorted
> buffer locks in earlier indexes. That really ain't gonna do, because in
> the case of an expression index, FormIndexDatum will execute nearly
> arbitrary user-defined code, which might well result in accesses to those
> indexes or others. What we'd have to do is refactor so that all the index
> tuple values get computed before we start to insert any of them. That
> doesn't seem impossible, but it implies a good deal more refactoring than
> has been done here.

We were proceeding on the basis that what I'd done, if deemed
acceptable in principle, could eventually be replaced by an
alternative value locking implementation that more or less similarly
extends the limited way in which value locking already occurs (i.e.
unique index enforcement's buffer locking), but without the downsides.
While I certainly appreciate your input, I still think that there is a
controversy about what implementation gets us the most useful
semantics, and I think we should now focus on resolving it. I am not
sure that Heikki's approach is functionally equivalent to mine. At the
very least, I think the trade-off of doing one or the other should be
well understood.

> Once we do that, I wonder if we couldn't get rid of the LWLockWeaken/
> Strengthen stuff. That scares the heck out of me; I think it's deadlock
> problems waiting to happen.

There are specific caveats around using those. I think that they could
be useful elsewhere, but are likely to only ever have a few clients.
As previously mentioned, the same semantics appear in other similar
locking primitives in other domains, so fwiw it really doesn't strike
me as all that controversial. I agree that their *usage* is not
acceptable as-is. I've only left the usage in the patch to give us
some basis for reasoning about the performance on mixed workloads for
comparative purposes. Perhaps I shouldn't have even done that, to
better focus reviewer attention on the semantics implied by each
implementation.

> Also, the lack of any doc updates makes it hard to review this. I can
> see that you don't want to touch the user-facing docs until the syntax
> is agreed on, but at the very least you ought to produce real updates
> for the indexam API spec, since you're changing that significantly.

I'll certainly do that in any future revision.

--
Peter Geoghegan