Re: Serializable lock consistency (was Re: CommitFest wrap-up)

On 17.12.2010 18:44, Florian Pflug wrote:
> On Dec17, 2010, at 16:49 , Heikki Linnakangas wrote:
>> On 15.12.2010 16:20, Florian Pflug wrote:
>>> On Dec14, 2010, at 15:01 , Robert Haas wrote:
>>>> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug<fgp@phlo.org>   wrote:
>>>>>> - serializable lock consistency - I am fairly certain this needs
>>>>>> rebasing.  I don't have time to deal with it right away.  That sucks,
>>>>>> because I think this is a really important change.
>>>>> I can try to find some time to update the patch if it suffers from bit-rot. Would that help?
>>>>
>>>> Yes!
>>>
>>> I've rebased the patch to the current HEAD, and re-run my FK concurrency test suite,
>>> available from https://github.com/fgp/fk_concurrency, to verify that things still work.
>>>
>>> I've also asserts to the callers of heap_{update,delete,lock_tuple} to verify (and document)
>>> that update_xmax may only be InvalidTransactionId if a lockcheck_snapshot is passed to
>>> heap_{update,delete,lock_tuple}.
>>>
>>> Finally, I've improved the explanation in src/backend/executor/README of how row locks and
>>> REPEATABLE READ transactions interact, and tried to state the guarantees provided by
>>> FOR SHARE and FOR UPDATE locks more precisely.
>>>
>>> I've published my work to https://github.com/fgp/postgres/tree/serializable_lock_consistency,
>>> and attached an updated patch. I'd be happy to give write access to that GIT repository
>>> to anyone who wants to help getting this committed.
>>
>> Here's some typo&  style fixes for that, also available at git://git.postgresql.org/git/users/heikki/postgres.git.
>
> Thanks! FYI, I've pulled these into https://github.com/fgp/postgres/tree/serializable_lock_consistency

I think this patch is in pretty good shape now.

The one thing I'm not too happy with is the API for 
heap_update/delete/lock_tuple. The return value is:

  * Normal, successful return value is HeapTupleMayBeUpdated, which
  * actually means we *did* update it.  Failure return codes are
  * HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
  * (the last only possible if wait == false).

And:

  * In the failure cases, the routine returns the tuple's t_ctid and t_max
  * in ctid and update_xmax.
  * If ctid is the same as t_self and update_xmax a valid transaction id,
  *	the tuple was deleted.
  * If ctid differs from t_self, the tuple was updated, ctid is the location
  *	of the replacement tuple and update_xmax is the updating 
transaction's xid.
  *	update_xmax must in this case be used to verify that the replacement 
tuple
  *	matched.
  * Otherwise, if ctid is the same as t_self and update_xmax is
  *	InvalidTransactionId, the tuple was neither replaced nor deleted, but
  *	locked by a transaction invisible to lockcheck_snapshot. This case can
  *	thus only arise if lockcheck_snapshot is a valid snapshot.

That's quite complicated. I think we should bite the bullet and add a 
couple of more return codes to explicitly tell the caller what happened. 
I propose:

HeapTupleMayBeUpdated- the tuple was actually updated (same as before)
HeapTupleSelfUpdated - the tuple was updated by a later command in same 
xact (same as before)
HeapTupleBeingUpdated - concurrent update in progress (same as before)
HeapTupleUpdated - the tuple was updated by another xact. *update_xmax 
and *ctid are set to point to the replacement tuple.
HeapTupleDeleted - the tuple was deleted by another xact
HeapTupleLocked - lockcheck_snapshot was given, and the tuple was locked 
by another xact

I'm not sure how to incorporate that into the current 
heap_delete/update/lock_tuple functions and HeapTupleSatisfiesUpdate. It 
would be nice to not copy-paste the logic to handle those into all three 
functions. Perhaps that common logic starting with the 
HeapTupleSatisfiesUpdate() call could be pulled into a common function.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com