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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
