Skip to content

Commit c0bd128

Browse files
alvherrealvherre
committed
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.
1 parent 12e41a5 commit c0bd128

File tree

3 files changed

+51
-13
lines changed

3 files changed

+51
-13
lines changed

‎src/backend/access/heap/heapam.c

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -184,9 +184,6 @@ static const int MultiXactStatusLock[MaxMultiXactStatus + 1] =
184184
/* Get the LockTupleMode for a given MultiXactStatus */
185185
#define TUPLOCK_from_mxstatus(status) \
186186
(MultiXactStatusLock[(status)])
187-
/* Get the is_update bit for a given MultiXactStatus */
188-
#define ISUPDATE_from_mxstatus(status) \
189-
((status) > MultiXactStatusForUpdate)
190187

191188
/* ----------------------------------------------------------------
192189
* heap support routines
@@ -2501,6 +2498,27 @@ compute_infobits(uint16 infomask, uint16 infomask2)
25012498
XLHL_KEYS_UPDATED : 0);
25022499
}
25032500

2501+
/*
2502+
* Given two versions of the same t_infomask for a tuple, compare them and
2503+
* return whether the relevant status for a tuple Xmax has changed. This is
2504+
* used after a buffer lock has been released and reacquired: we want to ensure
2505+
* that the tuple state continues to be the same it was when we previously
2506+
* examined it.
2507+
*
2508+
* Note the Xmax field itself must be compared separately.
2509+
*/
2510+
static inline bool
2511+
xmax_infomask_changed(uint16 new_infomask, uint16 old_infomask)
2512+
{
2513+
const uint16 interesting =
2514+
HEAP_XMAX_IS_MULTI | HEAP_XMAX_LOCK_ONLY | HEAP_LOCK_MASK;
2515+
2516+
if ((new_infomask & interesting) != (old_infomask & interesting))
2517+
return true;
2518+
2519+
return false;
2520+
}
2521+
25042522
/*
25052523
* heap_delete - delete a tuple
25062524
*
@@ -2634,7 +2652,7 @@ heap_delete(Relation relation, ItemPointer tid,
26342652
* update this tuple before we get to this point. Check for xmax
26352653
* change, and start over if so.
26362654
*/
2637-
if (!(tp.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
2655+
if (xmax_infomask_changed(tp.t_data->t_infomask, infomask) ||
26382656
!TransactionIdEquals(HeapTupleHeaderGetRawXmax(tp.t_data),
26392657
xwait))
26402658
goto l1;
@@ -2660,7 +2678,7 @@ heap_delete(Relation relation, ItemPointer tid,
26602678
* other xact could update this tuple before we get to this point.
26612679
* Check for xmax change, and start over if so.
26622680
*/
2663-
if ((tp.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
2681+
if (xmax_infomask_changed(tp.t_data->t_infomask, infomask) ||
26642682
!TransactionIdEquals(HeapTupleHeaderGetRawXmax(tp.t_data),
26652683
xwait))
26662684
goto l1;
@@ -3136,7 +3154,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
31363154
* update this tuple before we get to this point. Check for xmax
31373155
* change, and start over if so.
31383156
*/
3139-
if (!(oldtup.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
3157+
if (xmax_infomask_changed(oldtup.t_data->t_infomask, infomask) ||
31403158
!TransactionIdEquals(HeapTupleHeaderGetRawXmax(oldtup.t_data),
31413159
xwait))
31423160
goto l2;
@@ -3190,7 +3208,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
31903208
* recheck the locker; if someone else changed the tuple while
31913209
* we weren't looking, start over.
31923210
*/
3193-
if ((oldtup.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
3211+
if (xmax_infomask_changed(oldtup.t_data->t_infomask, infomask) ||
31943212
!TransactionIdEquals(
31953213
HeapTupleHeaderGetRawXmax(oldtup.t_data),
31963214
xwait))
@@ -3210,7 +3228,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
32103228
* some other xact could update this tuple before we get to
32113229
* this point. Check for xmax change, and start over if so.
32123230
*/
3213-
if ((oldtup.t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
3231+
if (xmax_infomask_changed(oldtup.t_data->t_infomask, infomask) ||
32143232
!TransactionIdEquals(
32153233
HeapTupleHeaderGetRawXmax(oldtup.t_data),
32163234
xwait))
@@ -4174,7 +4192,7 @@ heap_lock_tuple(Relation relation, HeapTuple tuple,
41744192
* over.
41754193
*/
41764194
LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE);
4177-
if (!(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
4195+
if (xmax_infomask_changed(tuple->t_data->t_infomask, infomask) ||
41784196
!TransactionIdEquals(HeapTupleHeaderGetRawXmax(tuple->t_data),
41794197
xwait))
41804198
{
@@ -4193,7 +4211,7 @@ heap_lock_tuple(Relation relation, HeapTuple tuple,
41934211
LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE);
41944212

41954213
/* if the xmax changed in the meantime, start over */
4196-
if ((tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
4214+
if (xmax_infomask_changed(tuple->t_data->t_infomask, infomask) ||
41974215
!TransactionIdEquals(
41984216
HeapTupleHeaderGetRawXmax(tuple->t_data),
41994217
xwait))
@@ -4257,7 +4275,7 @@ heap_lock_tuple(Relation relation, HeapTuple tuple,
42574275
* could update this tuple before we get to this point. Check
42584276
* for xmax change, and start over if so.
42594277
*/
4260-
if (!(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
4278+
if (xmax_infomask_changed(tuple->t_data->t_infomask, infomask) ||
42614279
!TransactionIdEquals(
42624280
HeapTupleHeaderGetRawXmax(tuple->t_data),
42634281
xwait))
@@ -4312,7 +4330,7 @@ heap_lock_tuple(Relation relation, HeapTuple tuple,
43124330
* some other xact could update this tuple before we get to
43134331
* this point. Check for xmax change, and start over if so.
43144332
*/
4315-
if ((tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI) ||
4333+
if (xmax_infomask_changed(tuple->t_data->t_infomask, infomask) ||
43164334
!TransactionIdEquals(
43174335
HeapTupleHeaderGetRawXmax(tuple->t_data),
43184336
xwait))

‎src/backend/access/transam/multixact.c

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -457,7 +457,7 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
457457
for (i = 0, j = 0; i < nmembers; i++)
458458
{
459459
if (TransactionIdIsInProgress(members[i].xid) ||
460-
((members[i].status > MultiXactStatusForUpdate) &&
460+
(ISUPDATE_from_mxstatus(members[i].status) &&
461461
TransactionIdDidCommit(members[i].xid)))
462462
{
463463
newMembers[j].xid = members[i].xid;
@@ -713,6 +713,22 @@ MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
713713
return multi;
714714
}
715715

716+
/* Verify that there is a single update Xid among the given members. */
717+
{
718+
int i;
719+
bool has_update = false;
720+
721+
for (i = 0; i < nmembers; i++)
722+
{
723+
if (ISUPDATE_from_mxstatus(members[i].status))
724+
{
725+
if (has_update)
726+
elog(ERROR, "new multixact has more than one updating member");
727+
has_update = true;
728+
}
729+
}
730+
}
731+
716732
/*
717733
* Assign the MXID and offsets range to use, and make sure there is space
718734
* in the OFFSETs and MEMBERs files. NB: this routine does

‎src/include/access/multixact.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ typedef enum
4848

4949
#define MaxMultiXactStatus MultiXactStatusUpdate
5050

51+
/* does a status value correspond to a tuple update? */
52+
#define ISUPDATE_from_mxstatus(status) \
53+
((status) > MultiXactStatusForUpdate)
54+
5155

5256
typedef struct MultiXactMember
5357
{

0 commit comments

Comments
 (0)