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

On Wed, Oct 9, 2013 at 9:30 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>>> * The syntax. I like the composability, and the way it's likely to
>>> become idiomatic to combine it with wCTEs. Others may not.
>>
>> I've actually lost track of what syntax you're proposing.
>
> I'm continuing to propose:
>
> INSERT...ON DUPLICATE KEY LOCK FOR UPDATE
>
> with a much less interesting variant that could be jettisoned:
>
> INSERT...ON DUPLICATE KEY IGNORE
>
> I'm also proposing extended RETURNING to make it work with this. So
> the basic idea is that within Postgres, the idiomatic way to correctly
> do upsert becomes something like:
>
> postgres=# with r as (
> insert into foo(a,b)
> values (5, '!'), (6, '@')
> on duplicate key lock for update
> returning rejects *
> )
> update foo set b = r.b from r where foo.a = r.a;

I can't claim to be enamored of this syntax.

>>> * The visibility hacks that V4 is likely to have. The fact that
>>> preserving the composable syntax may imply changes to
>>> HeapTupleSatisfiesMVCC() so that rows locked but with no currently
>>> visible version (under conventional rules) are visible to our snapshot
>>> by virtue of having been locked all the same (this only matters at
>>> read committed).
>>
>> I continue to think this is a bad idea.
>
> Fair enough.
>
> Is it just because of performance concerns? If so, that's probably not
> that hard to address. It either has a measurable impact on performance
> for a very unsympathetic benchmark or it doesn't. I guess that's the
> standard that I'll be held to, which is probably fair.

That's part of it; but I also think that HeapTupleSatisfiesMVCC() is a
pretty fundamental bit of the system that I am loathe to tamper with.
We can try to talk ourselves into believing that the definition change
will only affect this case, but I'm wary that there will be
unanticipated consequences, or simply that we'll find, after it's far
too late to do anything about it, that we don't particularly care for
the new semantics. It's probably an overstatement to say that I'll
oppose any whatsoever that touches the semantics of that function, but
not by much.

> Do you see the appeal of the composable syntax?

To some extent. It seems to me that what we're designing is a giant
grotty hack, albeit a convenient one. But if we're not really going
to get MERGE, I'm not sure how much good it is to try to pretend we've
got something general.

> I appreciate that it's odd that serializable transactions now have to
> worry about seeing something they shouldn't have seen (when they
> conclusively have to go lock a row version not current to their
> snapshot).

Surely that's never going to be acceptable. At read committed,
locking a version not current to the snapshot might be acceptable if
we hold our nose, but at any higher level I think we have to fail with
a serialization complaint.

> But that's simpler than any of the alternatives that I see.
> Does there really need to be a new snapshot type with one tiny
> difference that apparently doesn't actually affect conventional
> clients of MVCC snapshots?

I think that's the wrong way of thinking about it. If you're
introducing a new type of snapshot, or tinkering with the semantics of
an existing one, I think that's a reason to reject the patch straight
off. We should be looking for a design that doesn't require that. If
we can't find one, I'm not sure we should do this at all.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company