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

On Dec19, 2010, at 18:06 , Heikki Linnakangas wrote:
> I think this patch is in pretty good shape now.
Apart from the serious deficiency Robert found :-(

I'll still comment on your suggestions though, since
they'd also apply to the solution I suggested on the
other thread.

> The one thing I'm not too happy with is the API for heap_update/delete/lock_tuple. The return value is:
>
> <snipped comment citation>
>
> 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:

Yeah, it's a bit of a mess. On the other hand, heap_{update,delete,lock_tuple} are only called from very few places (simple_heap_update, simple_heap_delete, ExecUpdate, ExecLockRows and GetTupleForTrigger). Of these, only ExecUpdate and ExecLockRows care for update_xmax and ctid.

> 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

Hm, I'm not happy with HeapTupleMayBeUpdated meaning "The tuple was updated" while HeapTupleUpdated means "The tuple wasn't updated, a concurrent transaction beat us to it" seems less than ideal. On the whole, I'd much rather have a second enum, say HO_Result for heap operation result, instead of miss-using HTSU_Result for this. HO_Result would have the following possible values

HeapOperationCompleted - the tuple was updated/deleted/locked
HeapOperationSelfModified - the tuple was modified by a later command in the same xact. We don't distinguish the different cases here since none of the callers care.
HeapOperationBeingModified - the tuple was updated/deleted/locked (and the lock conflicts) by a transaction still in-progress.
HeapOperationConcurrentUpdate - the tuple was updated concurrently. *update_xmax and *ctid are set to point to the replacement tuple.
HeapOperationConcurrentDelete - the tuple was deleted concurrently.
HeapOperationConcurrentLock - the tuple was locked concurrently (only if lockcheck_snapshot was provided).

If we do want to keep heap_{update,delete,lock_tuple} result HTSU_Result, we could also add an output parameter of type HTSU_Failure with the possible values

HTSUConcurrentUpdate
HTSUConcurrentDelete
HTSUConcurrentLock

and set it accordingly if we return HeapTupleUpdated.

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

Hm, the logic in heap_lock_tuple is quite different from heap_delete and heap_update, since it needs to deal with share-mode lock acquisition. But for heap_{update,delete} unifying the logic should be possible.

best regards,
Florian Pflug