]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Repair two related errors in heap_lock_tuple: it was failing to recognize
authorTom Lane <tgl@sss.pgh.pa.us>
Fri, 17 Nov 2006 18:00:25 +0000 (18:00 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Fri, 17 Nov 2006 18:00:25 +0000 (18:00 +0000)
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.

src/backend/access/heap/heapam.c
src/backend/access/transam/multixact.c
src/backend/utils/time/tqual.c
src/include/access/multixact.h

index 4e9c0ff8bbae87542e925c0922a1ec3da8aa93eb..35ad3d88ac3c5a87351248501d6a3fdb4e680c87 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.200.2.2 2005/11/22 18:23:03 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.200.2.3 2006/11/17 18:00:25 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -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
                        {
index 28750d249073195db39345155a54d3ad818492c0..44861f323be1e194c57688e00f34534622e09dd4 100644 (file)
@@ -42,7 +42,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/backend/access/transam/multixact.c,v 1.11.2.2 2006/07/20 00:46:56 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/access/transam/multixact.c,v 1.11.2.3 2006/11/17 18:00:25 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -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.
index 27b712a22ae60ff61badfe2e83da4532f19dc786..4c1c8612c2c119cd441076e76de4f940c2ac9c4b 100644 (file)
@@ -32,7 +32,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/time/tqual.c,v 1.91.2.1 2005/11/22 18:23:25 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/time/tqual.c,v 1.91.2.2 2006/11/17 18:00:25 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -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,
index 02f2d601c50383aa7403a35e490f69a04e1b7e3c..93b2bc731543405674c3120813bfcb34eec47062 100644 (file)
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/access/multixact.h,v 1.7 2005/10/15 02:49:42 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/access/multixact.h,v 1.7.2.1 2006/11/17 18:00:25 tgl Exp $
  */
 #ifndef MULTIXACT_H
 #define MULTIXACT_H
@@ -41,6 +41,7 @@ typedef struct xl_multixact_create
 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);