Fix race when updating a tuple concurrently locked by another process

If a tuple is locked, and this lock is later upgraded either to an
update or to a stronger lock, and in the meantime some other process
tries to lock, update or delete the same tuple, it (the tuple) could end
up being updated twice, or having conflicting locks held.

The reason for this is that the second updater checks for a change in
Xmax value, or in the HEAP_XMAX_IS_MULTI infomask bit, after noticing
the first lock; and if there's a change, it restarts and re-evaluates
its ability to update the tuple.  But it neglected to check for changes
in lock strength or in lock-vs-update status when those two properties
stayed the same.  This would lead it to take the wrong decision and
continue with its own update, when in reality it shouldn't do so but
instead restart from the top.

This could lead to either an assertion failure much later (when a
multixact containing multiple updates is detected), or duplicate copies
of tuples.

To fix, make sure to compare the other relevant infomask bits alongside
the Xmax value and HEAP_XMAX_IS_MULTI bit, and restart from the top if
necessary.

Also, in the belt-and-suspenders spirit, add a check to
MultiXactCreateFromMembers that a multixact being created does not have
two or more members that are claimed to be updates.  This should protect
against other bugs that might cause similar bogus situations.

Backpatch to 9.3, where the possibility of multixacts containing updates
was introduced.  (In prior versions it was possible to have the tuple
lock upgraded from shared to exclusive, and an update would not restart
from the top; yet we're protected against a bug there because there's
always a sleep to wait for the locking transaction to complete before
continuing to do anything.  Really, the fact that tuple locks always
conflicted with concurrent updates is what protected against bugs here.)

Per report from Andrew Dunstan and Josh Berkus in thread at
http://www.postgresql.org/message-id/534C8B33.9050807@pgexperts.com

Bug analysis by Andres Freund.

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 1608f48c95c001bacb60dcbf0a84f91095b50c68..a8653e4a96cea00ad93cb95a17d1704808c142b1 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -184,9 +184,6 @@ static const int MultiXactStatusLock[MaxMultiXactStatus + 1] =
 /* Get the LockTupleMode for a given MultiXactStatus */
 #define TUPLOCK_from_mxstatus(status) \
                        (MultiXactStatusLock[(status)])
-/* Get the is_update bit for a given MultiXactStatus */
-#define ISUPDATE_from_mxstatus(status) \
-                       ((status) > MultiXactStatusForUpdate)
 
 /* ----------------------------------------------------------------
  *                                              heap support routines
@@ -2501,6 +2498,27 @@ compute_infobits(uint16 infomask, uint16 infomask2)
                 XLHL_KEYS_UPDATED : 0);
 }
 
+/*
+ * Given two versions of the same t_infomask for a tuple, compare them and
+ * return whether the relevant status for a tuple Xmax has changed.  This is
+ * used after a buffer lock has been released and reacquired: we want to ensure
+ * that the tuple state continues to be the same it was when we previously
+ * examined it.
+ *
+ * Note the Xmax field itself must be compared separately.
+ */
+static inline bool
+xmax_infomask_changed(uint16 new_infomask, uint16 old_infomask)
+{
+       const uint16    interesting =
+               HEAP_XMAX_IS_MULTI | HEAP_XMAX_LOCK_ONLY | HEAP_LOCK_MASK;
+
+       if ((new_infomask & interesting) != (old_infomask & interesting))
+               return true;
+
+       return false;
+}
+
 /*
  *     heap_delete - delete a tuple
  *
@@ -2634,7 +2652,7 @@ l1:
                         * update this tuple before we get to this point.  Check for xmax
                         * change, and start over if so.
                         */
-                       if (!(tp.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
+                       if (xmax_infomask_changed(tp.t_data->t_infomask, infomask) ||
                                !TransactionIdEquals(HeapTupleHeaderGetRawXmax(tp.t_data),
                                                                         xwait))
                                goto l1;
@@ -2660,7 +2678,7 @@ l1:
                         * other xact could update this tuple before we get to this point.
                         * Check for xmax change, and start over if so.
                         */
-                       if ((tp.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
+                       if (xmax_infomask_changed(tp.t_data->t_infomask, infomask) ||
                                !TransactionIdEquals(HeapTupleHeaderGetRawXmax(tp.t_data),
                                                                         xwait))
                                goto l1;
@@ -3136,7 +3154,7 @@ l2:
                         * update this tuple before we get to this point.  Check for xmax
                         * change, and start over if so.
                         */
-                       if (!(oldtup.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
+                       if (xmax_infomask_changed(oldtup.t_data->t_infomask, infomask) ||
                                !TransactionIdEquals(HeapTupleHeaderGetRawXmax(oldtup.t_data),
                                                                         xwait))
                                goto l2;
@@ -3190,7 +3208,7 @@ l2:
                                 * recheck the locker; if someone else changed the tuple while
                                 * we weren't looking, start over.
                                 */
-                               if ((oldtup.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
+                               if (xmax_infomask_changed(oldtup.t_data->t_infomask, infomask) ||
                                        !TransactionIdEquals(
                                                                        HeapTupleHeaderGetRawXmax(oldtup.t_data),
                                                                                 xwait))
@@ -3210,7 +3228,7 @@ l2:
                                 * some other xact could update this tuple before we get to
                                 * this point. Check for xmax change, and start over if so.
                                 */
-                               if ((oldtup.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
+                               if (xmax_infomask_changed(oldtup.t_data->t_infomask, infomask) ||
                                        !TransactionIdEquals(
                                                                        HeapTupleHeaderGetRawXmax(oldtup.t_data),
                                                                                 xwait))
@@ -4174,7 +4192,7 @@ l3:
                                                 * over.
                                                 */
                                                LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE);
-                                               if (!(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
+                                               if (xmax_infomask_changed(tuple->t_data->t_infomask, infomask) ||
                                                        !TransactionIdEquals(HeapTupleHeaderGetRawXmax(tuple->t_data),
                                                                                                 xwait))
                                                {
@@ -4193,7 +4211,7 @@ l3:
                                LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE);
 
                                /* if the xmax changed in the meantime, start over */
-                               if ((tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
+                               if (xmax_infomask_changed(tuple->t_data->t_infomask, infomask) ||
                                        !TransactionIdEquals(
                                                                        HeapTupleHeaderGetRawXmax(tuple->t_data),
                                                                                 xwait))
@@ -4257,7 +4275,7 @@ l3:
                                 * could update this tuple before we get to this point. Check
                                 * for xmax change, and start over if so.
                                 */
-                               if (!(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
+                               if (xmax_infomask_changed(tuple->t_data->t_infomask, infomask) ||
                                        !TransactionIdEquals(
                                                                        HeapTupleHeaderGetRawXmax(tuple->t_data),
                                                                                 xwait))
@@ -4312,7 +4330,7 @@ l3:
                                 * some other xact could update this tuple before we get to
                                 * this point.  Check for xmax change, and start over if so.
                                 */
-                               if ((tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
+                               if (xmax_infomask_changed(tuple->t_data->t_infomask, infomask) ||
                                        !TransactionIdEquals(
                                                                        HeapTupleHeaderGetRawXmax(tuple->t_data),
                                                                                 xwait))

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 541612e503ba1f82db181aac093bffa6b0af25b4..bacdbd64effe93741bfdeac877e38b76a4b57b65 100644 (file)
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -457,7 +457,7 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
        for (i = 0, j = 0; i < nmembers; i++)
        {
                if (TransactionIdIsInProgress(members[i].xid) ||
-                       ((members[i].status > MultiXactStatusForUpdate) &&
+                       (ISUPDATE_from_mxstatus(members[i].status) &&
                         TransactionIdDidCommit(members[i].xid)))
                {
                        newMembers[j].xid = members[i].xid;
@@ -713,6 +713,22 @@ MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
                return multi;
        }
 
+       /* Verify that there is a single update Xid among the given members. */
+       {
+               int                     i;
+               bool            has_update = false;
+
+               for (i = 0; i < nmembers; i++)
+               {
+                       if (ISUPDATE_from_mxstatus(members[i].status))
+                       {
+                               if (has_update)
+                                       elog(ERROR, "new multixact has more than one updating member");
+                               has_update = true;
+                       }
+               }
+       }
+
        /*
         * Assign the MXID and offsets range to use, and make sure there is space
         * in the OFFSETs and MEMBERs files.  NB: this routine does

diff --git a/src/include/access/multixact.h b/src/include/access/multixact.h
index 5f82907daccd1a061dcbd916ae2783ef221f8964..b9681090c060418fadc2538bd29b93db8b924ad0 100644 (file)
--- a/src/include/access/multixact.h
+++ b/src/include/access/multixact.h
@@ -48,6 +48,10 @@ typedef enum
 
 #define MaxMultiXactStatus MultiXactStatusUpdate
 
+/* does a status value correspond to a tuple update? */
+#define ISUPDATE_from_mxstatus(status) \
+                       ((status) > MultiXactStatusForUpdate)
+
 
 typedef struct MultiXactMember
 {