Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 1ce150b

Browse files
committed
Don't TransactionIdDidAbort in HeapTupleGetUpdateXid
It is dangerous to do so, because some code expects to be able to see what's the true Xmax even if it is aborted (particularly while traversing HOT chains). So don't do it, and instead rely on the callers to verify for abortedness, if necessary. Several race conditions and bugs fixed in the process. One isolation test changes the expected output due to these. This also reverts commit c235a6a, which is no longer necessary. Backpatch to 9.3, where this function was introduced. Andres Freund
1 parent 1df0122 commit 1ce150b

File tree

4 files changed

+75
-74
lines changed

4 files changed

+75
-74
lines changed

src/backend/access/heap/heapam.c

+10-11
Original file line numberDiff line numberDiff line change
@@ -3181,7 +3181,11 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
31813181
if (!HEAP_XMAX_IS_LOCKED_ONLY(oldtup.t_data->t_infomask))
31823182
update_xact = HeapTupleGetUpdateXid(oldtup.t_data);
31833183

3184-
/* there was no UPDATE in the MultiXact; or it aborted. */
3184+
/*
3185+
* There was no UPDATE in the MultiXact; or it aborted. No
3186+
* TransactionIdIsInProgress() call needed here, since we called
3187+
* MultiXactIdWait() above.
3188+
*/
31853189
if (!TransactionIdIsValid(update_xact) ||
31863190
TransactionIdDidAbort(update_xact))
31873191
can_continue = true;
@@ -5441,6 +5445,9 @@ GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
54415445
* Given a multixact Xmax and corresponding infomask, which does not have the
54425446
* HEAP_XMAX_LOCK_ONLY bit set, obtain and return the Xid of the updating
54435447
* transaction.
5448+
*
5449+
* Caller is expected to check the status of the updating transaction, if
5450+
* necessary.
54445451
*/
54455452
static TransactionId
54465453
MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask)
@@ -5465,19 +5472,11 @@ MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask)
54655472
for (i = 0; i < nmembers; i++)
54665473
{
54675474
/* Ignore lockers */
5468-
if (members[i].status == MultiXactStatusForKeyShare ||
5469-
members[i].status == MultiXactStatusForShare ||
5470-
members[i].status == MultiXactStatusForNoKeyUpdate ||
5471-
members[i].status == MultiXactStatusForUpdate)
5475+
if (!ISUPDATE_from_mxstatus(members[i].status))
54725476
continue;
54735477

5474-
/* ignore aborted transactions */
5475-
if (TransactionIdDidAbort(members[i].xid))
5476-
continue;
5477-
/* there should be at most one non-aborted updater */
5478+
/* there can be at most one updater */
54785479
Assert(update_xact == InvalidTransactionId);
5479-
Assert(members[i].status == MultiXactStatusNoKeyUpdate ||
5480-
members[i].status == MultiXactStatusUpdate);
54815480
update_xact = members[i].xid;
54825481
#ifndef USE_ASSERT_CHECKING
54835482

src/backend/access/heap/pruneheap.c

+6-16
Original file line numberDiff line numberDiff line change
@@ -479,22 +479,12 @@ heap_prune_chain(Relation relation, Buffer buffer, OffsetNumber rootoffnum,
479479
break;
480480

481481
case HEAPTUPLE_DELETE_IN_PROGRESS:
482-
{
483-
TransactionId xmax;
484-
485-
/*
486-
* This tuple may soon become DEAD. Update the hint field
487-
* so that the page is reconsidered for pruning in future.
488-
* If there was a MultiXactId updater, and it aborted after
489-
* HTSV checked, then we will get an invalid Xid here.
490-
* There is no need for future pruning of the page in that
491-
* case, so skip it.
492-
*/
493-
xmax = HeapTupleHeaderGetUpdateXid(htup);
494-
if (TransactionIdIsValid(xmax))
495-
heap_prune_record_prunable(prstate, xmax);
496-
}
497-
482+
/*
483+
* This tuple may soon become DEAD. Update the hint field
484+
* so that the page is reconsidered for pruning in future.
485+
*/
486+
heap_prune_record_prunable(prstate,
487+
HeapTupleHeaderGetUpdateXid(htup));
498488
break;
499489

500490
case HEAPTUPLE_LIVE:

src/backend/utils/time/tqual.c

+51-42
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,9 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
223223
TransactionId xmax;
224224

225225
xmax = HeapTupleGetUpdateXid(tuple);
226-
if (!TransactionIdIsValid(xmax))
227-
return true;
226+
227+
/* not LOCKED_ONLY, so it has to have an xmax */
228+
Assert(TransactionIdIsValid(xmax));
228229

229230
/* updating subtransaction must have aborted */
230231
if (!TransactionIdIsCurrentTransactionId(xmax))
@@ -277,14 +278,17 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
277278
return true;
278279

279280
xmax = HeapTupleGetUpdateXid(tuple);
280-
if (!TransactionIdIsValid(xmax))
281-
return true;
281+
282+
/* not LOCKED_ONLY, so it has to have an xmax */
283+
Assert(TransactionIdIsValid(xmax));
284+
282285
if (TransactionIdIsCurrentTransactionId(xmax))
283286
return false;
284287
if (TransactionIdIsInProgress(xmax))
285288
return true;
286289
if (TransactionIdDidCommit(xmax))
287290
return false;
291+
/* it must have aborted or crashed */
288292
return true;
289293
}
290294

@@ -497,8 +501,9 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
497501
TransactionId xmax;
498502

499503
xmax = HeapTupleGetUpdateXid(tuple);
500-
if (!TransactionIdIsValid(xmax))
501-
return HeapTupleMayBeUpdated;
504+
505+
/* not LOCKED_ONLY, so it has to have an xmax */
506+
Assert(TransactionIdIsValid(xmax));
502507

503508
/* updating subtransaction must have aborted */
504509
if (!TransactionIdIsCurrentTransactionId(xmax))
@@ -573,14 +578,9 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
573578
}
574579

575580
xmax = HeapTupleGetUpdateXid(tuple);
576-
if (!TransactionIdIsValid(xmax))
577-
{
578-
if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
579-
return HeapTupleBeingUpdated;
580581

581-
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
582-
return HeapTupleMayBeUpdated;
583-
}
582+
/* not LOCKED_ONLY, so it has to have an xmax */
583+
Assert(TransactionIdIsValid(xmax));
584584

585585
if (TransactionIdIsCurrentTransactionId(xmax))
586586
{
@@ -590,13 +590,18 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
590590
return HeapTupleInvisible; /* updated before scan started */
591591
}
592592

593-
if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
593+
if (TransactionIdIsInProgress(xmax))
594594
return HeapTupleBeingUpdated;
595595

596596
if (TransactionIdDidCommit(xmax))
597597
return HeapTupleUpdated;
598+
599+
/* no member, even just a locker, alive anymore */
600+
if (!MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
601+
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
602+
InvalidTransactionId);
603+
598604
/* it must have aborted or crashed */
599-
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
600605
return HeapTupleMayBeUpdated;
601606
}
602607

@@ -722,8 +727,9 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
722727
TransactionId xmax;
723728

724729
xmax = HeapTupleGetUpdateXid(tuple);
725-
if (!TransactionIdIsValid(xmax))
726-
return true;
730+
731+
/* not LOCKED_ONLY, so it has to have an xmax */
732+
Assert(TransactionIdIsValid(xmax));
727733

728734
/* updating subtransaction must have aborted */
729735
if (!TransactionIdIsCurrentTransactionId(xmax))
@@ -780,8 +786,10 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
780786
return true;
781787

782788
xmax = HeapTupleGetUpdateXid(tuple);
783-
if (!TransactionIdIsValid(xmax))
784-
return true;
789+
790+
/* not LOCKED_ONLY, so it has to have an xmax */
791+
Assert(TransactionIdIsValid(xmax));
792+
785793
if (TransactionIdIsCurrentTransactionId(xmax))
786794
return false;
787795
if (TransactionIdIsInProgress(xmax))
@@ -791,6 +799,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
791799
}
792800
if (TransactionIdDidCommit(xmax))
793801
return false;
802+
/* it must have aborted or crashed */
794803
return true;
795804
}
796805

@@ -915,8 +924,9 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
915924
TransactionId xmax;
916925

917926
xmax = HeapTupleGetUpdateXid(tuple);
918-
if (!TransactionIdIsValid(xmax))
919-
return true;
927+
928+
/* not LOCKED_ONLY, so it has to have an xmax */
929+
Assert(TransactionIdIsValid(xmax));
920930

921931
/* updating subtransaction must have aborted */
922932
if (!TransactionIdIsCurrentTransactionId(xmax))
@@ -975,8 +985,10 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
975985
Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask));
976986

977987
xmax = HeapTupleGetUpdateXid(tuple);
978-
if (!TransactionIdIsValid(xmax))
979-
return true;
988+
989+
/* not LOCKED_ONLY, so it has to have an xmax */
990+
Assert(TransactionIdIsValid(xmax));
991+
980992
if (TransactionIdIsCurrentTransactionId(xmax))
981993
{
982994
if (HeapTupleHeaderGetCmax(tuple) >= snapshot->curcid)
@@ -993,6 +1005,7 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
9931005
return true; /* treat as still in progress */
9941006
return false;
9951007
}
1008+
/* it must have aborted or crashed */
9961009
return true;
9971010
}
9981011

@@ -1191,8 +1204,10 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
11911204
Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask));
11921205

11931206
xmax = HeapTupleGetUpdateXid(tuple);
1194-
if (!TransactionIdIsValid(xmax))
1195-
return HEAPTUPLE_LIVE;
1207+
1208+
/* not LOCKED_ONLY, so it has to have an xmax */
1209+
Assert(TransactionIdIsValid(xmax));
1210+
11961211
if (TransactionIdIsInProgress(xmax))
11971212
return HEAPTUPLE_DELETE_IN_PROGRESS;
11981213
else if (TransactionIdDidCommit(xmax))
@@ -1205,8 +1220,10 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
12051220
Assert(!(tuple->t_infomask & HEAP_XMAX_COMMITTED));
12061221

12071222
xmax = HeapTupleGetUpdateXid(tuple);
1208-
if (!TransactionIdIsValid(xmax))
1209-
return HEAPTUPLE_LIVE;
1223+
1224+
/* not LOCKED_ONLY, so it has to have an xmax */
1225+
Assert(TransactionIdIsValid(xmax));
1226+
12101227
/* multi is not running -- updating xact cannot be */
12111228
Assert(!TransactionIdIsInProgress(xmax));
12121229
if (TransactionIdDidCommit(xmax))
@@ -1216,22 +1233,13 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
12161233
else
12171234
return HEAPTUPLE_DEAD;
12181235
}
1219-
else
1220-
{
1221-
/*
1222-
* Not in Progress, Not Committed, so either Aborted or crashed.
1223-
*/
1224-
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
1225-
return HEAPTUPLE_LIVE;
1226-
}
12271236

12281237
/*
1229-
* Deleter committed, but perhaps it was recent enough that some open
1230-
* transactions could still see the tuple.
1238+
* Not in Progress, Not Committed, so either Aborted or crashed.
1239+
* Remove the Xmax.
12311240
*/
1232-
1233-
/* Otherwise, it's dead and removable */
1234-
return HEAPTUPLE_DEAD;
1241+
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
1242+
return HEAPTUPLE_LIVE;
12351243
}
12361244

12371245
if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED))
@@ -1474,8 +1482,9 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
14741482

14751483
/* ... but if it's a multi, then perhaps the updating Xid aborted. */
14761484
xmax = HeapTupleGetUpdateXid(tuple);
1477-
if (!TransactionIdIsValid(xmax)) /* shouldn't happen .. */
1478-
return true;
1485+
1486+
/* not LOCKED_ONLY, so it has to have an xmax */
1487+
Assert(TransactionIdIsValid(xmax));
14791488

14801489
if (TransactionIdIsCurrentTransactionId(xmax))
14811490
return false;

src/test/isolation/expected/delete-abort-savept.out

+8-5
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,11 @@ key value
2323
step s1svp: SAVEPOINT f;
2424
step s1d: DELETE FROM foo;
2525
step s1r: ROLLBACK TO f;
26-
step s2l: SELECT * FROM foo FOR UPDATE; <waiting ...>
27-
step s1c: COMMIT;
28-
step s2l: <... completed>
26+
step s2l: SELECT * FROM foo FOR UPDATE;
2927
key value
3028

3129
1 1
30+
step s1c: COMMIT;
3231
step s2c: COMMIT;
3332

3433
starting permutation: s1l s1svp s1d s1r s2l s2c s1c
@@ -39,8 +38,12 @@ key value
3938
step s1svp: SAVEPOINT f;
4039
step s1d: DELETE FROM foo;
4140
step s1r: ROLLBACK TO f;
42-
step s2l: SELECT * FROM foo FOR UPDATE; <waiting ...>
43-
invalid permutation detected
41+
step s2l: SELECT * FROM foo FOR UPDATE;
42+
key value
43+
44+
1 1
45+
step s2c: COMMIT;
46+
step s1c: COMMIT;
4447

4548
starting permutation: s1l s1svp s1d s2l s1r s1c s2c
4649
step s1l: SELECT * FROM foo FOR KEY SHARE;

0 commit comments

Comments
 (0)