Re: [HACKERS] 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:
>
>
>
> 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
Re: [HACKERS] 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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Serializable lock consistency (was Re: CommitFest wrap-up)
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 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
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
[HACKERS] Serializable lock consistency (was Re: CommitFest wrap-up)
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 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.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index a397bad..3b12aca 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2034,8 +2034,8 @@ simple_heap_insert(Relation relation, HeapTuple tup)
* cid - delete command ID (used for visibility test, and stored into
* cmax if successful)
* wait - true if should wait for any conflicting update to commit/abort
- * lockcheck_snapshot - if not InvalidSnapshot, report the tuple as updated if it
- * was once locked by a transaction not visible under this snapshot
+ * lockcheck_snapshot - if not InvalidSnapshot, report the tuple as updated
+ * if it was once locked by a transaction not visible under this snapshot
*
* Normal, successful return value is HeapTupleMayBeUpdated, which
* actually means we did delete it. Failure return codes are
@@ -2046,14 +2046,14 @@ simple_heap_insert(Relation relation, HeapTuple tup)
* 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 differes from t_self, the tuple was updated, ctid is the location
- * of the replacement tupe and update_xmax is the updating transaction's xid.
+ * 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
- * InvalidTranactionId, 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.
+ * 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.
*/
HTSU_Result
heap_delete(Relation relation, ItemPointer tid,
@@ -2180,7 +2180,8 @@ l1:
result = HeapTupleUpdated;
}
- /* If the tuple was updated, we report the updating transaction's
+ /*
+ * If the tuple was updated, we report the updating transaction's
* xid in update_xmax. Otherwise, we must check that it wasn't
* locked by a transaction invisible to lockcheck_snapshot before
* continuing.
@@ -2368,9 +2369,9 @@ simple_heap_delete(Relation relation, ItemPointer tid)
* update_xmax - output parameter, used only for failure case (see below)
* cid - update command ID (used for visibility test, and stored into
* cmax/cmin if successful)
+ * wait - true if should wait for any conflicting update to commit/abort
* lockcheck_snapshot - if not InvalidSnapshot, report the tuple as updated
* if it was once locked by a transaction not visible under this snapshot
- * wait - true if should wait for any conflicting update to commit/abort
*
* Normal, successful return value is HeapTupleMayBeUpdated, which
* actually means we *did* update it. Failure return codes are
@@ -2387,14 +2388,14 @@ simple_heap_delete(Relation relation, ItemPointer tid)
* 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 differes from t_self, the tuple was updated, ctid is the location
- * of the replacement tupe and update_xmax is the updating transaction's xid.
+ * If ctid differs from t_self, the tuple was updated, ctid is th
