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<f...@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.
--
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 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_update(Relation relation, ItemPointer otid, HeapTuple newtup,
@@ -2551,7 +2552,8 @@ l2:
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.
@@ -3070,14 +3072,14 @@ simple_heap_update(Relation relation, ItemPointer otid, 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.
*
* NOTES: because the shared-memory lock table is of finite size, but users
* could reasonably want to lock large numbers of tuples, we do not rely on
@@ -3296,7 +3298,8 @@ l3:
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.
diff --git a/src/backend/executor/README b/src/backend/executor/README
index ab70f40..a0aa0ba 100644
--- a/src/backend/executor/README
+++ b/src/backend/executor/README
@@ -209,12 +209,12 @@ level READ COMMITTED), this is not satisfactory. The RI triggers for example
take a FOR SHARE lock on a parent row before allowing a child row to be
inserted and verify that deleting a parent row leaves no orphaned children
behind before allowing the delete to occur. From within READ COMMITTED
-transactions, blocking upon a delete or a parent row until all lockers have
+transactions, blocking upon a delete of the parent row until all lockers have
finished is sufficient to guarantee that this check finds any potential
orphan, since the check will be executed with a up-to-date snapshot to which
the locking transaction's changes are visible. For isolation level REPEATABLE
-READ and higher, however, is not true, since these will continue to use their
-old snapshot and hence miss newly inserted rows.
+READ and higher, however, it's not enough, since these will continue to use
+their old snapshot and hence miss newly inserted rows.
Such transactions therefore treat a FOR SHARE or FOR UPDATE lock on a tuple
the same as an actual update during UPDATE and SELECT FOR UPDATE. They are
@@ -222,7 +222,7 @@ thus aborted when trying to UPDATE or FOR UPDATE lock a row that was FOR SHARE
or FOR UPDATE locked by a concurrent transaction.
Note that the behavior is not totally symmetric - locking a row FOR UPDATE
-that was concurrently locked FOR SHARE abort with a serialization error, while
+that was concurrently locked FOR SHARE aborts with a serialization error, while
FOR SHARE locking a row that was concurrently locked FOR UPDATE will succeed
(after the locking transaction has committed or aborted, of course, and assume
that the FOR UPDATE wasn't followed by an actual UPDATE). This is hard to
@@ -237,7 +237,7 @@ are undesirable, the former because of the increased number of serialization
errors it'd cause and the latter because obtaining a FOR UPDATE lock is
supposed to prevent a future update from failing.
-The guarantees provided by the different lock strengths are thus
+The guarantees provided by the different lock strengths are thus:
1) Acquiring a FOR SHARE lock guarantees, for all modifications of the tuple
from within any transaction, that all statements run *after* the modifying
@@ -252,12 +252,13 @@ transaction that at one point obtained a FOR SHARE lock on the tuple. For
isolation level REPEATABLE READ and above, even the statement itself will see
these changes.
-With isolation level REPEATABLE READ and above, a serialization error is raised if one of these guarantees is violated.
+With isolation level REPEATABLE READ and above, a serialization error is
+raised if one of these guarantees is violated.
Implementation-wise, heap_update(), heap_delete() and heap_lock_tuple() take a
-parameter lockcheck_snapshot(), and report a tuple as updated if it was locked
-by a transaction not visible to lockcheck_snapshot(). The implementation
-depends on the fact that if one transaction invisible to lockcheck_snapshop
+parameter lockcheck_snapshot, and report a tuple as updated if it was locked
+by a transaction not visible to lockcheck_snapshot. The implementation
+depends on the fact that if one transaction invisible to lockcheck_snapshot
locks the tuple, every future locker must still be running by the time the
first locker commits or aborts, and will thus surely be invisible to the
lockcheck_snapshot if the original locker was.
diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c
index 330e5f0..4298aed 100644
--- a/src/backend/executor/nodeLockRows.c
+++ b/src/backend/executor/nodeLockRows.c
@@ -21,8 +21,8 @@
#include "postgres.h"
-#include "access/xact.h"
#include "access/transam.h"
+#include "access/xact.h"
#include "executor/executor.h"
#include "executor/nodeLockRows.h"
#include "storage/bufmgr.h"
@@ -160,9 +160,10 @@ lnext:
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
- /* In case of a per-query snapshot, we didn't pass
+ /*
+ * In case of a per-query snapshot, we didn't pass
* a lockcheck_snapshot to heap_update() and can
- * thus expect a tuple to have a valid xmax here
+ * thus expect a tuple to have a valid xmax here.
*/
Assert(TransactionIdIsValid(update_xmax));
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 4e32c18..1f610b7 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -37,8 +37,8 @@
#include "postgres.h"
-#include "access/xact.h"
#include "access/transam.h"
+#include "access/xact.h"
#include "commands/trigger.h"
#include "executor/executor.h"
#include "executor/nodeModifyTable.h"
@@ -396,9 +396,10 @@ ldelete:;
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
- /* In case of a per-query snapshot, we didn't pass
+ /*
+ * In case of a per-query snapshot, we didn't pass
* a lockcheck_snapshot to heap_update() and can
- * thus expect a tuple to have a valid xmax here
+ * thus expect a tuple to have a valid xmax here.
*/
Assert(TransactionIdIsValid(update_xmax));
if (!ItemPointerEquals(tupleid, &update_ctid))
@@ -650,9 +651,10 @@ lreplace:;
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
- /* In case of a per-query snapshot, we didn't pass
+ /*
+ * In case of a per-query snapshot, we didn't pass
* a lockcheck_snapshot to heap_update() and can
- * thus expect a tuple to have a valid xmax here
+ * thus expect a tuple to have a valid xmax here.
*/
Assert(TransactionIdIsValid(update_xmax));
if (!ItemPointerEquals(tupleid, &update_ctid))
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index cd277ab..93df895 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -25,9 +25,9 @@
*/
#include "postgres.h"
+#include "access/multixact.h"
#include "access/transam.h"
#include "access/xact.h"
-#include "access/multixact.h"
#include "storage/proc.h"
#include "storage/procarray.h"
#include "utils/memutils.h"
@@ -130,7 +130,7 @@ GetTransactionSnapshot(void)
/*
* We must store the oldest visible multi xact *before* taking the
* serializable snapshot. Otherwise HeapSatisfiesLockersVisible in
- * heapam.c will be in trouble, since it depends on being able to
+ * tqual.c will be in trouble, since it depends on being able to
* inspect all multi xact ids which might contain xids invisible to
* the serializable snapshot.
*/
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index f49a112..6230ab5 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -1247,8 +1247,8 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin,
* Returns false if one of the tuple's lockers committed but
* isn't visible according to lockcheck_snapshot, excluding subtransactions
* of the current transaction.
- * Assumes that all locking transaction either committed or aborted,
- * but aren't still in progress.
+ * Assumes that all locking transactions either committed or aborted,
+ * and aren't still in progress.
*/
bool
HeapSatisfiesLockersVisible(HeapTupleHeader tuple, Snapshot lockcheck_snapshot)
@@ -1261,13 +1261,13 @@ HeapSatisfiesLockersVisible(HeapTupleHeader tuple, Snapshot lockcheck_snapshot)
/*
* If the tuple was locked, we now check whether the locking
* transaction(s) are visible under lockcheck_snapshot. If
- * they aren't, we pretend that the tuple was updated.
+ * they aren't, return false.
*/
if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
- TransactionId* xids;
- int xids_l = GetMultiXactIdMembers(HeapTupleHeaderGetXmax(tuple), &xids);
-
+ TransactionId *xids;
+ int xids_l = GetMultiXactIdMembers(HeapTupleHeaderGetXmax(tuple),
+ &xids);
if (xids_l < 1)
{
/*
@@ -1287,7 +1287,7 @@ HeapSatisfiesLockersVisible(HeapTupleHeader tuple, Snapshot lockcheck_snapshot)
else
{
int i;
- for (i = 0; i < xids_l; ++i)
+ for (i = 0; i < xids_l; i++)
{
/* Ignore our own subtransactions */
if (TransactionIdIsCurrentTransactionId(xids[i]))
@@ -1295,7 +1295,7 @@ HeapSatisfiesLockersVisible(HeapTupleHeader tuple, Snapshot lockcheck_snapshot)
/*
* We expect to be called after the locking transactions'
- * fates have been decided
+ * fates have been decided.
*/
Assert(!TransactionIdIsInProgress(xids[i]));
@@ -1317,7 +1317,10 @@ HeapSatisfiesLockersVisible(HeapTupleHeader tuple, Snapshot lockcheck_snapshot)
if (TransactionIdIsCurrentTransactionId(xid))
return true;
- /* We expect to be called after the locking transactions' fates have been decided */
+ /*
+ * We expect to be called after the locking transactions' fates
+ * have been decided.
+ */
Assert(!TransactionIdIsInProgress(xid));
/* Locker must either be visible or have aborted */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
