diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 68c14925ed..4e1cec4403 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3164,7 +3164,11 @@ l2: if (!HEAP_XMAX_IS_LOCKED_ONLY(oldtup.t_data->t_infomask)) 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) || TransactionIdDidAbort(update_xact)) can_continue = true; @@ -5424,6 +5428,9 @@ GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask, * 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 * transaction. + * + * Caller is expected to check the status of the updating transaction, if + * necessary. */ static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask) @@ -5448,19 +5455,11 @@ MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask) for (i = 0; i < nmembers; i++) { /* Ignore lockers */ - if (members[i].status == MultiXactStatusForKeyShare || - members[i].status == MultiXactStatusForShare || - members[i].status == MultiXactStatusForNoKeyUpdate || - members[i].status == MultiXactStatusForUpdate) + if (!ISUPDATE_from_mxstatus(members[i].status)) continue; - /* ignore aborted transactions */ - if (TransactionIdDidAbort(members[i].xid)) - continue; - /* there should be at most one non-aborted updater */ + /* there can be at most one updater */ Assert(update_xact == InvalidTransactionId); - Assert(members[i].status == MultiXactStatusNoKeyUpdate || - members[i].status == MultiXactStatusUpdate); update_xact = members[i].xid; #ifndef USE_ASSERT_CHECKING diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 638ce26ef4..8f0c02d9c2 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -466,22 +466,12 @@ heap_prune_chain(Relation relation, Buffer buffer, OffsetNumber rootoffnum, break; case HEAPTUPLE_DELETE_IN_PROGRESS: - { - TransactionId xmax; - - /* - * This tuple may soon become DEAD. Update the hint field - * 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. - * There is no need for future pruning of the page in that - * case, so skip it. - */ - xmax = HeapTupleHeaderGetUpdateXid(htup); - if (TransactionIdIsValid(xmax)) - heap_prune_record_prunable(prstate, xmax); - } - + /* + * This tuple may soon become DEAD. Update the hint field + * so that the page is reconsidered for pruning in future. + */ + heap_prune_record_prunable(prstate, + HeapTupleHeaderGetUpdateXid(htup)); break; case HEAPTUPLE_LIVE: diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c index 70923bd4ac..4d63b1c186 100644 --- a/src/backend/utils/time/tqual.c +++ b/src/backend/utils/time/tqual.c @@ -222,8 +222,9 @@ HeapTupleSatisfiesSelf(HeapTupleHeader tuple, Snapshot snapshot, Buffer buffer) TransactionId xmax; 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 */ if (!TransactionIdIsCurrentTransactionId(xmax)) @@ -276,14 +277,17 @@ HeapTupleSatisfiesSelf(HeapTupleHeader tuple, Snapshot snapshot, Buffer buffer) return true; xmax = HeapTupleGetUpdateXid(tuple); - if (!TransactionIdIsValid(xmax)) - return true; + + /* not LOCKED_ONLY, so it has to have an xmax */ + Assert(TransactionIdIsValid(xmax)); + if (TransactionIdIsCurrentTransactionId(xmax)) return false; if (TransactionIdIsInProgress(xmax)) return true; if (TransactionIdDidCommit(xmax)) return false; + /* it must have aborted or crashed */ return true; } @@ -690,8 +694,9 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid, TransactionId xmax; 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 */ if (!TransactionIdIsCurrentTransactionId(xmax)) @@ -766,14 +771,9 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid, } xmax = HeapTupleGetUpdateXid(tuple); - if (!TransactionIdIsValid(xmax)) - { - if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple))) - return HeapTupleBeingUpdated; - SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); - return HeapTupleMayBeUpdated; - } + /* not LOCKED_ONLY, so it has to have an xmax */ + Assert(TransactionIdIsValid(xmax)); if (TransactionIdIsCurrentTransactionId(xmax)) { @@ -783,13 +783,18 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid, return HeapTupleInvisible; /* updated before scan started */ } - if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple))) + if (TransactionIdIsInProgress(xmax)) return HeapTupleBeingUpdated; if (TransactionIdDidCommit(xmax)) 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 */ - SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); return HeapTupleMayBeUpdated; } @@ -911,8 +916,9 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple, Snapshot snapshot, TransactionId xmax; 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 */ if (!TransactionIdIsCurrentTransactionId(xmax)) @@ -969,8 +975,10 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple, Snapshot snapshot, return true; xmax = HeapTupleGetUpdateXid(tuple); - if (!TransactionIdIsValid(xmax)) - return true; + + /* not LOCKED_ONLY, so it has to have an xmax */ + Assert(TransactionIdIsValid(xmax)); + if (TransactionIdIsCurrentTransactionId(xmax)) return false; if (TransactionIdIsInProgress(xmax)) @@ -980,6 +988,7 @@ HeapTupleSatisfiesDirty(HeapTupleHeader tuple, Snapshot snapshot, } if (TransactionIdDidCommit(xmax)) return false; + /* it must have aborted or crashed */ return true; } @@ -1104,8 +1113,9 @@ HeapTupleSatisfiesMVCC(HeapTupleHeader tuple, Snapshot snapshot, TransactionId xmax; 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 */ if (!TransactionIdIsCurrentTransactionId(xmax)) @@ -1164,8 +1174,10 @@ HeapTupleSatisfiesMVCC(HeapTupleHeader tuple, Snapshot snapshot, Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)); 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 (HeapTupleHeaderGetCmax(tuple) >= snapshot->curcid) @@ -1182,6 +1194,7 @@ HeapTupleSatisfiesMVCC(HeapTupleHeader tuple, Snapshot snapshot, return true; /* treat as still in progress */ return false; } + /* it must have aborted or crashed */ return true; } @@ -1376,8 +1389,10 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin, Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)); 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)) return HEAPTUPLE_DELETE_IN_PROGRESS; else if (TransactionIdDidCommit(xmax)) @@ -1390,8 +1405,10 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin, Assert(!(tuple->t_infomask & HEAP_XMAX_COMMITTED)); 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 */ Assert(!TransactionIdIsInProgress(xmax)); if (TransactionIdDidCommit(xmax)) @@ -1401,22 +1418,13 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin, else return HEAPTUPLE_DEAD; } - else - { - /* - * Not in Progress, Not Committed, so either Aborted or crashed. - */ - SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); - return HEAPTUPLE_LIVE; - } /* - * Deleter committed, but perhaps it was recent enough that some open - * transactions could still see the tuple. + * Not in Progress, Not Committed, so either Aborted or crashed. + * Remove the Xmax. */ - - /* Otherwise, it's dead and removable */ - return HEAPTUPLE_DEAD; + SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); + return HEAPTUPLE_LIVE; } if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED)) @@ -1655,8 +1663,9 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple) /* ... but if it's a multi, then perhaps the updating Xid aborted. */ 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)) return false; diff --git a/src/test/isolation/expected/delete-abort-savept.out b/src/test/isolation/expected/delete-abort-savept.out index 3420cf47d7..5b8c444728 100644 --- a/src/test/isolation/expected/delete-abort-savept.out +++ b/src/test/isolation/expected/delete-abort-savept.out @@ -23,12 +23,11 @@ key value step s1svp: SAVEPOINT f; step s1d: DELETE FROM foo; step s1r: ROLLBACK TO f; -step s2l: SELECT * FROM foo FOR UPDATE; -step s1c: COMMIT; -step s2l: <... completed> +step s2l: SELECT * FROM foo FOR UPDATE; key value 1 1 +step s1c: COMMIT; step s2c: COMMIT; starting permutation: s1l s1svp s1d s1r s2l s2c s1c @@ -39,8 +38,12 @@ key value step s1svp: SAVEPOINT f; step s1d: DELETE FROM foo; step s1r: ROLLBACK TO f; -step s2l: SELECT * FROM foo FOR UPDATE; -invalid permutation detected +step s2l: SELECT * FROM foo FOR UPDATE; +key value + +1 1 +step s2c: COMMIT; +step s1c: COMMIT; starting permutation: s1l s1svp s1d s2l s1r s1c s2c step s1l: SELECT * FROM foo FOR KEY SHARE;