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 c235a6a589b, which is no longer necessary.

Backpatch to 9.3, where this function was introduced.

Andres Freund

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 68c14925ed53cd550dfb7752052413bb7da1db77..4e1cec440335fb49795e17995948b8148073a93b 100644 (file)
--- 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 638ce26ef4ea04bc5e161e75f1606326527011c7..8f0c02d9c2c28e3e88349e2e41d50dd844b223ab 100644 (file)
--- 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 70923bd4ac145eb7442f4870402225f1adcf0c14..4d63b1c1864a17cfb0ae24d08f689c0d7ef5134b 100644 (file)
--- 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 3420cf47d77334c6c8936d125c43c41d913f76bf..5b8c4447284e85ace3bca7406557211e0b450514 100644 (file)
--- 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; <waiting ...>
-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; <waiting ...>
-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;