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

On Wed, Jan 15, 2014 at 11:02 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> It might just be a matter of:
>
> @@ -186,6 +186,13 @@ ExecLockHeapTupleForUpdateSpec(EState *estate,
> switch (test)
> {
> case HeapTupleInvisible:
> + /*
> + * Tuple may have originated from this command, in which case it's
> + * already locked
> + */
> + if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmin(tuple.t_data))
> &&
> + HeapTupleHeaderGetCmin(tuple.t_data) == estate->es_output_cid)
> + return true;
> /* Tuple became invisible; try again */
> if (IsolationUsesXactSnapshot())
> ereport(ERROR,

I think we need to give this some more thought. I have not addressed
the implications for MVCC snapshots here. I think that I'll need to
raise a WARNING along the lines of "your snapshot isn't going to
consider the locked tuple visible because the same command inserted
it", or perhaps even raise an ERROR regardless of isolation level
(although note that I'm not suggesting that we raise an ERROR in the
event of receiving HeapTupleInvisible from heap_lock_tuple()/HTSU()
for other reasons, which *is* possible, nor am I suggesting that later
commands of the same xact would ever see this ERROR). I'm comfortable
with the idea of what you might loosely describe as a "READ COMMITTED
mode serialization failure" here, because this case is so much more
narrow than the other case I've proposed making a special exception to
the general semantics of MVCC snapshots to accommodate (i.e. the case
where a tuple is locked from an xact logically still-in-progress to
our snapshot in RC mode).

I think I'll be happy to declare that usage of the feature that hits
this issue is somewhere between questionable and wrong. It probably
isn't worth making another, similar HTSMVCC exception for this case.
But ISTM that we still have to do *something* other than simply credit
users with taking care to avoid tripping up on this.

--
Peter Geoghegan