From: Tom Lane Date: Fri, 17 Nov 2006 18:00:25 +0000 (+0000) Subject: Repair two related errors in heap_lock_tuple: it was failing to recognize X-Git-Url: http://waps.l3s.uni-hannover.de/gitweb/?a=commitdiff_plain;h=9f5b0c32a1651254cd2b76ad659663c7c076f6e9;p=users%2Fbernd%2Fpostgres.git Repair two related errors in heap_lock_tuple: it was failing to recognize cases where we already hold the desired lock "indirectly", either via membership in a MultiXact or because the lock was originally taken by a different subtransaction of the current transaction. These cases must be accounted for to avoid needless deadlocks and/or inappropriate replacement of an exclusive lock with a shared lock. Per report from Clarence Gardner and subsequent investigation. --- diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 491a52856c..e61b92c445 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -2080,6 +2080,8 @@ heap_lock_tuple(Relation relation, HeapTuple tuple, Buffer *buffer, ItemId lp; PageHeader dp; TransactionId xid; + TransactionId xmax; + uint16 old_infomask; uint16 new_infomask; LOCKMODE tuple_lock_type; bool have_tuple_lock = false; @@ -2118,6 +2120,25 @@ l3: LockBuffer(*buffer, BUFFER_LOCK_UNLOCK); + /* + * If we wish to acquire share lock, and the tuple is already + * share-locked by a multixact that includes any subtransaction of the + * current top transaction, then we effectively hold the desired lock + * already. We *must* succeed without trying to take the tuple lock, + * else we will deadlock against anyone waiting to acquire exclusive + * lock. We don't need to make any state changes in this case. + */ + if (mode == LockTupleShared && + (infomask & HEAP_XMAX_IS_MULTI) && + MultiXactIdIsCurrent((MultiXactId) xwait)) + { + Assert(infomask & HEAP_XMAX_SHARED_LOCK); + /* Probably can't hold tuple lock here, but may as well check */ + if (have_tuple_lock) + UnlockTuple(relation, tid, tuple_lock_type); + return HeapTupleMayBeUpdated; + } + /* * Acquire tuple lock to establish our priority for the tuple. * LockTuple will release us when we are next-in-line for the tuple. @@ -2255,6 +2276,35 @@ l3: return result; } + /* + * We might already hold the desired lock (or stronger), possibly under + * a different subtransaction of the current top transaction. If so, + * there is no need to change state or issue a WAL record. We already + * handled the case where this is true for xmax being a MultiXactId, + * so now check for cases where it is a plain TransactionId. + * + * Note in particular that this covers the case where we already hold + * exclusive lock on the tuple and the caller only wants shared lock. + * It would certainly not do to give up the exclusive lock. + */ + xmax = HeapTupleHeaderGetXmax(tuple->t_data); + old_infomask = tuple->t_data->t_infomask; + + if (!(old_infomask & (HEAP_XMAX_INVALID | + HEAP_XMAX_COMMITTED | + HEAP_XMAX_IS_MULTI)) && + (mode == LockTupleShared ? + (old_infomask & HEAP_IS_LOCKED) : + (old_infomask & HEAP_XMAX_EXCL_LOCK)) && + TransactionIdIsCurrentTransactionId(xmax)) + { + LockBuffer(*buffer, BUFFER_LOCK_UNLOCK); + /* Probably can't hold tuple lock here, but may as well check */ + if (have_tuple_lock) + UnlockTuple(relation, tid, tuple_lock_type); + return HeapTupleMayBeUpdated; + } + /* * Compute the new xmax and infomask to store into the tuple. Note we do * not modify the tuple just yet, because that would leave it in the wrong @@ -2262,19 +2312,14 @@ l3: */ xid = GetCurrentTransactionId(); - new_infomask = tuple->t_data->t_infomask; - - new_infomask &= ~(HEAP_XMAX_COMMITTED | - HEAP_XMAX_INVALID | - HEAP_XMAX_IS_MULTI | - HEAP_IS_LOCKED | - HEAP_MOVED); + new_infomask = old_infomask & ~(HEAP_XMAX_COMMITTED | + HEAP_XMAX_INVALID | + HEAP_XMAX_IS_MULTI | + HEAP_IS_LOCKED | + HEAP_MOVED); if (mode == LockTupleShared) { - TransactionId xmax = HeapTupleHeaderGetXmax(tuple->t_data); - uint16 old_infomask = tuple->t_data->t_infomask; - /* * If this is the first acquisition of a shared lock in the current * transaction, set my per-backend OldestMemberMXactId setting. We can @@ -2315,32 +2360,13 @@ l3: } else if (TransactionIdIsInProgress(xmax)) { - if (TransactionIdEquals(xmax, xid)) - { - /* - * If the old locker is ourselves, we'll just mark the - * tuple again with our own TransactionId. However we - * have to consider the possibility that we had exclusive - * rather than shared lock before --- if so, be careful to - * preserve the exclusivity of the lock. - */ - if (!(old_infomask & HEAP_XMAX_SHARED_LOCK)) - { - new_infomask &= ~HEAP_XMAX_SHARED_LOCK; - new_infomask |= HEAP_XMAX_EXCL_LOCK; - mode = LockTupleExclusive; - } - } - else - { - /* - * If the Xmax is a valid TransactionId, then we need to - * create a new MultiXactId that includes both the old - * locker and our own TransactionId. - */ - xid = MultiXactIdCreate(xmax, xid); - new_infomask |= HEAP_XMAX_IS_MULTI; - } + /* + * If the XMAX is a valid TransactionId, then we need to + * create a new MultiXactId that includes both the old + * locker and our own TransactionId. + */ + xid = MultiXactIdCreate(xmax, xid); + new_infomask |= HEAP_XMAX_IS_MULTI; } else { diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index fedc8dcb17..6a5ee2dcbb 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -365,7 +365,6 @@ bool MultiXactIdIsRunning(MultiXactId multi) { TransactionId *members; - TransactionId myXid; int nmembers; int i; @@ -379,12 +378,14 @@ MultiXactIdIsRunning(MultiXactId multi) return false; } - /* checking for myself is cheap */ - myXid = GetTopTransactionId(); - + /* + * Checking for myself is cheap compared to looking in shared memory, + * so first do the equivalent of MultiXactIdIsCurrent(). This is not + * needed for correctness, it's just a fast path. + */ for (i = 0; i < nmembers; i++) { - if (TransactionIdEquals(members[i], myXid)) + if (TransactionIdIsCurrentTransactionId(members[i])) { debug_elog3(DEBUG2, "IsRunning: I (%d) am running!", i); pfree(members); @@ -415,6 +416,44 @@ MultiXactIdIsRunning(MultiXactId multi) return false; } +/* + * MultiXactIdIsCurrent + * Returns true if the current transaction is a member of the MultiXactId. + * + * We return true if any live subtransaction of the current top-level + * transaction is a member. This is appropriate for the same reason that a + * lock held by any such subtransaction is globally equivalent to a lock + * held by the current subtransaction: no such lock could be released without + * aborting this subtransaction, and hence releasing its locks. So it's not + * necessary to add the current subxact to the MultiXact separately. + */ +bool +MultiXactIdIsCurrent(MultiXactId multi) +{ + bool result = false; + TransactionId *members; + int nmembers; + int i; + + nmembers = GetMultiXactIdMembers(multi, &members); + + if (nmembers < 0) + return false; + + for (i = 0; i < nmembers; i++) + { + if (TransactionIdIsCurrentTransactionId(members[i])) + { + result = true; + break; + } + } + + pfree(members); + + return result; +} + /* * MultiXactIdSetOldestMember * Save the oldest MultiXactId this transaction could be a member of. diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c index 12d136153d..7367f39d69 100644 --- a/src/backend/utils/time/tqual.c +++ b/src/backend/utils/time/tqual.c @@ -506,7 +506,10 @@ HeapTupleSatisfiesToast(HeapTupleHeader tuple, Buffer buffer) * HeapTupleUpdated: The tuple was updated by a committed transaction. * * HeapTupleBeingUpdated: The tuple is being updated by an in-progress - * transaction other than the current transaction. + * transaction other than the current transaction. (Note: this includes + * the case where the tuple is share-locked by a MultiXact, even if the + * MultiXact includes the current transaction. Callers that want to + * distinguish that case must test for it themselves.) */ HTSU_Result HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid, diff --git a/src/include/access/multixact.h b/src/include/access/multixact.h new file mode 100644 index 0000000000..67d64729be --- /dev/null +++ b/src/include/access/multixact.h @@ -0,0 +1,69 @@ +/* + * multixact.h + * + * PostgreSQL multi-transaction-log manager + * + * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * $PostgreSQL$ + */ +#ifndef MULTIXACT_H +#define MULTIXACT_H + +#include "access/xlog.h" + +#define InvalidMultiXactId ((MultiXactId) 0) +#define FirstMultiXactId ((MultiXactId) 1) + +#define MultiXactIdIsValid(multi) ((multi) != InvalidMultiXactId) + +/* ---------------- + * multixact-related XLOG entries + * ---------------- + */ + +#define XLOG_MULTIXACT_ZERO_OFF_PAGE 0x00 +#define XLOG_MULTIXACT_ZERO_MEM_PAGE 0x10 +#define XLOG_MULTIXACT_CREATE_ID 0x20 + +typedef struct xl_multixact_create +{ + MultiXactId mid; /* new MultiXact's ID */ + MultiXactOffset moff; /* its starting offset in members file */ + int32 nxids; /* number of member XIDs */ + TransactionId xids[1]; /* VARIABLE LENGTH ARRAY */ +} xl_multixact_create; + +#define MinSizeOfMultiXactCreate offsetof(xl_multixact_create, xids) + + +extern MultiXactId MultiXactIdCreate(TransactionId xid1, TransactionId xid2); +extern MultiXactId MultiXactIdExpand(MultiXactId multi, TransactionId xid); +extern bool MultiXactIdIsRunning(MultiXactId multi); +extern bool MultiXactIdIsCurrent(MultiXactId multi); +extern void MultiXactIdWait(MultiXactId multi); +extern bool ConditionalMultiXactIdWait(MultiXactId multi); +extern void MultiXactIdSetOldestMember(void); +extern int GetMultiXactIdMembers(MultiXactId multi, TransactionId **xids); + +extern void AtEOXact_MultiXact(void); + +extern Size MultiXactShmemSize(void); +extern void MultiXactShmemInit(void); +extern void BootStrapMultiXact(void); +extern void StartupMultiXact(void); +extern void ShutdownMultiXact(void); +extern void MultiXactGetCheckptMulti(bool is_shutdown, + MultiXactId *nextMulti, + MultiXactOffset *nextMultiOffset); +extern void CheckPointMultiXact(void); +extern void MultiXactSetNextMXact(MultiXactId nextMulti, + MultiXactOffset nextMultiOffset); +extern void MultiXactAdvanceNextMXact(MultiXactId minMulti, + MultiXactOffset minMultiOffset); + +extern void multixact_redo(XLogRecPtr lsn, XLogRecord *record); +extern void multixact_desc(char *buf, uint8 xl_info, char *rec); + +#endif /* MULTIXACT_H */