open-dsi
/
postgres
mirror of https://github.com/postgres/postgres
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Don't TransactionIdDidAbort in HeapTupleGetUpdateXid

Browse Source 
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 c235a6a589, which is no longer necessary.

Backpatch to 9.3, where this function was introduced.

Andres Freund
pull/6/head
Alvaro Herrera 12 years ago
parent 1df0122daa
commit 1ce150b7bb
4 changed files with 75 additions and 74 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 21
      src/backend/access/heap/heapam.c
  2. 16
      src/backend/access/heap/pruneheap.c
  3. 89
      src/backend/utils/time/tqual.c
  4. 13
      src/test/isolation/expected/delete-abort-savept.out

21
src/backend/access/heap/heapam.c
Unescape View File

@ -3181,7 +3181,11 @@ l2:
if (!HEAP_XMAX_IS_LOCKED_ONLY(oldtup.t_data->t_infomask)) if (!HEAP_XMAX_IS_LOCKED_ONLY(oldtup.t_data->t_infomask))
update_xact = HeapTupleGetUpdateXid(oldtup.t_data); update_xact = HeapTupleGetUpdateXid(oldtup.t_data);
/* there was no UPDATE in the MultiXact; or it aborted. */ /*
* There was no UPDATE in the MultiXact; or it aborted. No
* TransactionIdIsInProgress() call needed here, since we called
* MultiXactIdWait() above.
*/
if (!TransactionIdIsValid(update_xact) || if (!TransactionIdIsValid(update_xact) ||
TransactionIdDidAbort(update_xact)) TransactionIdDidAbort(update_xact))
can_continue = true; can_continue = true;
@ -5441,6 +5445,9 @@ GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
* Given a multixact Xmax and corresponding infomask, which does not have the * Given a multixact Xmax and corresponding infomask, which does not have the
* HEAP_XMAX_LOCK_ONLY bit set, obtain and return the Xid of the updating * HEAP_XMAX_LOCK_ONLY bit set, obtain and return the Xid of the updating
* transaction. * transaction.
*
* Caller is expected to check the status of the updating transaction, if
* necessary.
*/ */
static TransactionId static TransactionId
MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask) MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask)
@ -5465,19 +5472,11 @@ MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask)
for (i = 0; i < nmembers; i++) for (i = 0; i < nmembers; i++)
{ {
/* Ignore lockers */ /* Ignore lockers */
if (members[i].status == MultiXactStatusForKeyShare || if (!ISUPDATE_from_mxstatus(members[i].status))
members[i].status == MultiXactStatusForShare ||
members[i].status == MultiXactStatusForNoKeyUpdate ||
members[i].status == MultiXactStatusForUpdate)
continue; continue;
/* ignore aborted transactions */ /* there can be at most one updater */
if (TransactionIdDidAbort(members[i].xid))
continue;
/* there should be at most one non-aborted updater */
Assert(update_xact == InvalidTransactionId); Assert(update_xact == InvalidTransactionId);
Assert(members[i].status == MultiXactStatusNoKeyUpdate ||
members[i].status == MultiXactStatusUpdate);
update_xact = members[i].xid; update_xact = members[i].xid;
#ifndef USE_ASSERT_CHECKING #ifndef USE_ASSERT_CHECKING

16
src/backend/access/heap/pruneheap.c
Unescape View File

@ -479,22 +479,12 @@ heap_prune_chain(Relation relation, Buffer buffer, OffsetNumber rootoffnum,
break; break;
case HEAPTUPLE_DELETE_IN_PROGRESS: case HEAPTUPLE_DELETE_IN_PROGRESS:
{
TransactionId xmax;
/* /*
* This tuple may soon become DEAD. Update the hint field * This tuple may soon become DEAD. Update the hint field
* so that the page is reconsidered for pruning in future. * so that the page is reconsidered for pruning in future.
* If there was a MultiXactId updater, and it aborted after */
* HTSV checked, then we will get an invalid Xid here. heap_prune_record_prunable(prstate,
* There is no need for future pruning of the page in that HeapTupleHeaderGetUpdateXid(htup));
* case, so skip it.
*/
xmax = HeapTupleHeaderGetUpdateXid(htup);
if (TransactionIdIsValid(xmax))
heap_prune_record_prunable(prstate, xmax);
}
break; break;
case HEAPTUPLE_LIVE: case HEAPTUPLE_LIVE:

89
src/backend/utils/time/tqual.c
Unescape View File

@ -223,8 +223,9 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
TransactionId xmax; TransactionId xmax;
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
return true; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
/* updating subtransaction must have aborted */ /* updating subtransaction must have aborted */
if (!TransactionIdIsCurrentTransactionId(xmax)) if (!TransactionIdIsCurrentTransactionId(xmax))
@ -277,14 +278,17 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
return true; return true;
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
return true; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
if (TransactionIdIsCurrentTransactionId(xmax)) if (TransactionIdIsCurrentTransactionId(xmax))
return false; return false;
if (TransactionIdIsInProgress(xmax)) if (TransactionIdIsInProgress(xmax))
return true; return true;
if (TransactionIdDidCommit(xmax)) if (TransactionIdDidCommit(xmax))
return false; return false;
/* it must have aborted or crashed */
return true; return true;
} }
@ -497,8 +501,9 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
TransactionId xmax; TransactionId xmax;
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
return HeapTupleMayBeUpdated; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
/* updating subtransaction must have aborted */ /* updating subtransaction must have aborted */
if (!TransactionIdIsCurrentTransactionId(xmax)) if (!TransactionIdIsCurrentTransactionId(xmax))
@ -573,14 +578,9 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
} }
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
{
if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
return HeapTupleBeingUpdated;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); /* not LOCKED_ONLY, so it has to have an xmax */
return HeapTupleMayBeUpdated; Assert(TransactionIdIsValid(xmax));
}
if (TransactionIdIsCurrentTransactionId(xmax)) if (TransactionIdIsCurrentTransactionId(xmax))
{ {
@ -590,13 +590,18 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
return HeapTupleInvisible; /* updated before scan started */ return HeapTupleInvisible; /* updated before scan started */
} }
if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple))) if (TransactionIdIsInProgress(xmax))
return HeapTupleBeingUpdated; return HeapTupleBeingUpdated;
if (TransactionIdDidCommit(xmax)) if (TransactionIdDidCommit(xmax))
return HeapTupleUpdated; return HeapTupleUpdated;
/* no member, even just a locker, alive anymore */
if (!MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
InvalidTransactionId);
/* it must have aborted or crashed */ /* it must have aborted or crashed */
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
return HeapTupleMayBeUpdated; return HeapTupleMayBeUpdated;
} }
@ -722,8 +727,9 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
TransactionId xmax; TransactionId xmax;
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
return true; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
/* updating subtransaction must have aborted */ /* updating subtransaction must have aborted */
if (!TransactionIdIsCurrentTransactionId(xmax)) if (!TransactionIdIsCurrentTransactionId(xmax))
@ -780,8 +786,10 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
return true; return true;
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
return true; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
if (TransactionIdIsCurrentTransactionId(xmax)) if (TransactionIdIsCurrentTransactionId(xmax))
return false; return false;
if (TransactionIdIsInProgress(xmax)) if (TransactionIdIsInProgress(xmax))
@ -791,6 +799,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
} }
if (TransactionIdDidCommit(xmax)) if (TransactionIdDidCommit(xmax))
return false; return false;
/* it must have aborted or crashed */
return true; return true;
} }
@ -915,8 +924,9 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
TransactionId xmax; TransactionId xmax;
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
return true; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
/* updating subtransaction must have aborted */ /* updating subtransaction must have aborted */
if (!TransactionIdIsCurrentTransactionId(xmax)) if (!TransactionIdIsCurrentTransactionId(xmax))
@ -975,8 +985,10 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)); Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask));
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
return true; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
if (TransactionIdIsCurrentTransactionId(xmax)) if (TransactionIdIsCurrentTransactionId(xmax))
{ {
if (HeapTupleHeaderGetCmax(tuple) >= snapshot->curcid) if (HeapTupleHeaderGetCmax(tuple) >= snapshot->curcid)
@ -993,6 +1005,7 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
return true; /* treat as still in progress */ return true; /* treat as still in progress */
return false; return false;
} }
/* it must have aborted or crashed */
return true; return true;
} }
@ -1191,8 +1204,10 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)); Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask));
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
return HEAPTUPLE_LIVE; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
if (TransactionIdIsInProgress(xmax)) if (TransactionIdIsInProgress(xmax))
return HEAPTUPLE_DELETE_IN_PROGRESS; return HEAPTUPLE_DELETE_IN_PROGRESS;
else if (TransactionIdDidCommit(xmax)) else if (TransactionIdDidCommit(xmax))
@ -1205,8 +1220,10 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
Assert(!(tuple->t_infomask & HEAP_XMAX_COMMITTED)); Assert(!(tuple->t_infomask & HEAP_XMAX_COMMITTED));
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
return HEAPTUPLE_LIVE; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
/* multi is not running -- updating xact cannot be */ /* multi is not running -- updating xact cannot be */
Assert(!TransactionIdIsInProgress(xmax)); Assert(!TransactionIdIsInProgress(xmax));
if (TransactionIdDidCommit(xmax)) if (TransactionIdDidCommit(xmax))
@ -1216,24 +1233,15 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
else else
return HEAPTUPLE_DEAD; return HEAPTUPLE_DEAD;
} }
else
{
/* /*
* Not in Progress, Not Committed, so either Aborted or crashed. * Not in Progress, Not Committed, so either Aborted or crashed.
* Remove the Xmax.
*/ */
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
return HEAPTUPLE_LIVE; return HEAPTUPLE_LIVE;
} }
/*
* Deleter committed, but perhaps it was recent enough that some open
* transactions could still see the tuple.
*/
/* Otherwise, it's dead and removable */
return HEAPTUPLE_DEAD;
}
if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED)) if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED))
{ {
if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple))) if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
@ -1474,8 +1482,9 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
/* ... but if it's a multi, then perhaps the updating Xid aborted. */ /* ... but if it's a multi, then perhaps the updating Xid aborted. */
xmax = HeapTupleGetUpdateXid(tuple); xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax)) /* shouldn't happen .. */
return true; /* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
if (TransactionIdIsCurrentTransactionId(xmax)) if (TransactionIdIsCurrentTransactionId(xmax))
return false; return false;

13
src/test/isolation/expected/delete-abort-savept.out
Unescape View File

@ -23,12 +23,11 @@ key value
step s1svp: SAVEPOINT f; step s1svp: SAVEPOINT f;
step s1d: DELETE FROM foo; step s1d: DELETE FROM foo;
step s1r: ROLLBACK TO f; step s1r: ROLLBACK TO f;
step s2l: SELECT * FROM foo FOR UPDATE; <waiting ...> step s2l: SELECT * FROM foo FOR UPDATE;
step s1c: COMMIT;
step s2l: <... completed>
key value key value
1 1 1 1
step s1c: COMMIT;
step s2c: COMMIT; step s2c: COMMIT;
starting permutation: s1l s1svp s1d s1r s2l s2c s1c starting permutation: s1l s1svp s1d s1r s2l s2c s1c
@ -39,8 +38,12 @@ key value
step s1svp: SAVEPOINT f; step s1svp: SAVEPOINT f;
step s1d: DELETE FROM foo; step s1d: DELETE FROM foo;
step s1r: ROLLBACK TO f; step s1r: ROLLBACK TO f;
step s2l: SELECT * FROM foo FOR UPDATE; <waiting ...> step s2l: SELECT * FROM foo FOR UPDATE;
invalid permutation detected key value
1 1
step s2c: COMMIT;
step s1c: COMMIT;
starting permutation: s1l s1svp s1d s2l s1r s1c s2c starting permutation: s1l s1svp s1d s2l s1r s1c s2c
step s1l: SELECT * FROM foo FOR KEY SHARE; step s1l: SELECT * FROM foo FOR KEY SHARE;

Write Preview
Loading…
Cancel
Save