Fix freezing of a dead HOT-updated tuple

Vacuum calls page-level HOT prune to remove dead HOT tuples before doing
liveness checks (HeapTupleSatisfiesVacuum) on the remaining tuples.  But
concurrent transaction commit/abort may turn DEAD some of the HOT tuples
that survived the prune, before HeapTupleSatisfiesVacuum tests them.
This happens to activate the code that decides to freeze the tuple ...
which resuscitates it, duplicating data.

(This is especially bad if there's any unique constraints, because those
are now internally violated due to the duplicate entries, though you
won't know until you try to REINDEX or dump/restore the table.)

One possible fix would be to simply skip doing anything to the tuple,
and hope that the next HOT prune would remove it.  But there is a
problem: if the tuple is older than freeze horizon, this would leave an
unfrozen XID behind, and if no HOT prune happens to clean it up before
the containing pg_clog segment is truncated away, it'd later cause an
error when the XID is looked up.

Fix the problem by having the tuple freezing routines cope with the
situation: don't freeze the tuple (and keep it dead).  In the cases that
the XID is older than the freeze age, set the HEAP_XMAX_COMMITTED flag
so that there is no need to look up the XID in pg_clog later on.

An isolation test is included, authored by Michael Paquier, loosely
based on Daniel Wood's original reproducer.  It only tests one
particular scenario, though, not all the possible ways for this problem
to surface; it be good to have a more reliable way to test this more
fully, but it'd require more work.
In message https://postgr.es/m/20170911140103[email protected]
I outlined another test case (more closely matching Dan Wood's) that
exposed a few more ways for the problem to occur.

Backpatch all the way back to 9.3, where this problem was introduced by
multixact juggling.  In branches 9.3 and 9.4, this includes a backpatch
of commit e5ff9fefcd50 (of 9.5 era), since the original is not
correctable without matching the coding pattern in 9.5 up.

Reported-by: Daniel Wood
Diagnosed-by: Daniel Wood
Reviewed-by: Yi Wen Wong, Michaël Paquier
Discussion: https://postgr.es/m/E5711E62-8FDF-4DCA-A888-C200BF6B5742@amazon.com

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f5db3c775c3417e3f7923d92ee5756adb5f68942..e00dc6c1ca90cce5fd29a7e8ab5613d048f0179c 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5489,14 +5489,23 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
            Assert(TransactionIdIsValid(xid));
 
            /*
-            * If the xid is older than the cutoff, it has to have aborted,
-            * otherwise the tuple would have gotten pruned away.
+            * The updating transaction cannot possibly be still running, but
+            * verify whether it has committed, and request to set the
+            * COMMITTED flag if so.  (We normally don't see any tuples in
+            * this state, because they are removed by page pruning before we
+            * try to freeze the page; but this can happen if the updating
+            * transaction commits after the page is pruned but before
+            * HeapTupleSatisfiesVacuum).
             */
            if (TransactionIdPrecedes(xid, cutoff_xid))
            {
-               Assert(!TransactionIdDidCommit(xid));
-               *flags |= FRM_INVALIDATE_XMAX;
-               xid = InvalidTransactionId;     /* not strictly necessary */
+               if (TransactionIdDidCommit(xid))
+                   *flags = FRM_MARK_COMMITTED | FRM_RETURN_IS_XID;
+               else
+               {
+                   *flags |= FRM_INVALIDATE_XMAX;
+                   xid = InvalidTransactionId; /* not strictly necessary */
+               }
            }
            else
            {
@@ -5567,18 +5576,17 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
 
            /*
             * It's an update; should we keep it?  If the transaction is known
-            * aborted then it's okay to ignore it, otherwise not.  However,
-            * if the Xid is older than the cutoff_xid, we must remove it.
-            * Note that such an old updater cannot possibly be committed,
-            * because HeapTupleSatisfiesVacuum would have returned
-            * HEAPTUPLE_DEAD and we would not be trying to freeze the tuple.
-            *
-            * Note the TransactionIdDidAbort() test is just an optimization
-            * and not strictly necessary for correctness.
+            * aborted or crashed then it's okay to ignore it, otherwise not.
             *
             * As with all tuple visibility routines, it's critical to test
-            * TransactionIdIsInProgress before the transam.c routines,
+            * TransactionIdIsInProgress before TransactionIdDidCommit,
             * because of race conditions explained in detail in tqual.c.
+            *
+            * We normally don't see committed updating transactions earlier
+            * than the cutoff xid, because they are removed by page pruning
+            * before we try to freeze the page; but it can happen if the
+            * updating transaction commits after the page is pruned but
+            * before HeapTupleSatisfiesVacuum.
             */
            if (TransactionIdIsCurrentTransactionId(xid) ||
                TransactionIdIsInProgress(xid))
@@ -5586,46 +5594,33 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
                Assert(!TransactionIdIsValid(update_xid));
                update_xid = xid;
            }
-           else if (!TransactionIdDidAbort(xid))
+           else if (TransactionIdDidCommit(xid))
            {
                /*
-                * Test whether to tell caller to set HEAP_XMAX_COMMITTED
-                * while we have the Xid still in cache.  Note this can only
-                * be done if the transaction is known not running.
+                * The transaction committed, so we can tell caller to set
+                * HEAP_XMAX_COMMITTED.  (We can only do this because we know
+                * the transaction is not running.)
                 */
-               if (TransactionIdDidCommit(xid))
-                   update_committed = true;
                Assert(!TransactionIdIsValid(update_xid));
+               update_committed = true;
                update_xid = xid;
            }
 
+           /*
+            * Not in progress, not committed -- must be aborted or crashed;
+            * we can ignore it.
+            */
+
            /*
             * If we determined that it's an Xid corresponding to an update
             * that must be retained, additionally add it to the list of
-            * members of the new Multis, in case we end up using that.  (We
+            * members of the new Multi, in case we end up using that.  (We
             * might still decide to use only an update Xid and not a multi,
             * but it's easier to maintain the list as we walk the old members
             * list.)
-            *
-            * It is possible to end up with a very old updater Xid that
-            * crashed and thus did not mark itself as aborted in pg_clog.
-            * That would manifest as a pre-cutoff Xid.  Make sure to ignore
-            * it.
             */
            if (TransactionIdIsValid(update_xid))
-           {
-               if (!TransactionIdPrecedes(update_xid, cutoff_xid))
-               {
-                   newmembers[nnewmembers++] = members[i];
-               }
-               else
-               {
-                   /* cannot have committed: would be HEAPTUPLE_DEAD */
-                   Assert(!TransactionIdDidCommit(update_xid));
-                   update_xid = InvalidTransactionId;
-                   update_committed = false;
-               }
-           }
+               newmembers[nnewmembers++] = members[i];
        }
        else
        {
@@ -5692,7 +5687,10 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
  *
  * It is assumed that the caller has checked the tuple with
  * HeapTupleSatisfiesVacuum() and determined that it is not HEAPTUPLE_DEAD
- * (else we should be removing the tuple, not freezing it).
+ * (else we should be removing the tuple, not freezing it).  However, note
+ * that we don't remove HOT tuples even if they are dead, and it'd be incorrect
+ * to freeze them (because that would make them visible), so we mark them as
+ * update-committed, and needing further freezing later on.
  *
  * NB: cutoff_xid *must* be <= the current global xmin, to ensure that any
  * XID older than it could neither be running nor seen as running by any
@@ -5803,7 +5801,18 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid,
    else if (TransactionIdIsNormal(xid) &&
             TransactionIdPrecedes(xid, cutoff_xid))
    {
-       freeze_xmax = true;
+       /*
+        * Must freeze regular XIDs older than the cutoff.  We must not freeze
+        * a HOT-updated tuple, though; doing so would bring it back to life.
+        */
+       if (HeapTupleHeaderIsHotUpdated(tuple))
+       {
+           frz->t_infomask |= HEAP_XMAX_COMMITTED;
+           changed = true;
+           /* must not freeze */
+       }
+       else
+           freeze_xmax = true;
    }
 
    if (freeze_xmax)

diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 6ece3591e4f189b7019b193a7f64b3060bcb959f..abef418106840d2dd53f04dfe79160dc00ca04a6 100644 (file)
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -1678,15 +1678,15 @@ lazy_record_dead_tuple(LVRelStats *vacrelstats,
                       ItemPointer itemptr)
 {
    /*
-    * The array shouldn't overflow under normal behavior, but perhaps it
-    * could if we are given a really small maintenance_work_mem. In that
-    * case, just forget the last few tuples (we'll get 'em next time).
+    * The array must never overflow, since we rely on all deletable tuples
+    * being removed; inability to remove a tuple might cause an old XID to
+    * persist beyond the freeze limit, which could be disastrous later on.
     */
-   if (vacrelstats->num_dead_tuples < vacrelstats->max_dead_tuples)
-   {
-       vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;
-       vacrelstats->num_dead_tuples++;
-   }
+   if (vacrelstats->num_dead_tuples >= vacrelstats->max_dead_tuples)
+       elog(ERROR, "dead tuple array overflow");
+
+   vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;
+   vacrelstats->num_dead_tuples++;
 }
 
 /*

diff --git a/src/test/isolation/expected/freeze-the-dead.out b/src/test/isolation/expected/freeze-the-dead.out
new file mode 100644 (file)
index 0000000..dd04561
--- /dev/null
+++ b/src/test/isolation/expected/freeze-the-dead.out
@@ -0,0 +1,101 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_update s1_commit s1_vacuum s2_key_share s2_commit
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s1_commit: COMMIT;
+step s1_vacuum: VACUUM FREEZE tab_freeze;
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s2_commit: COMMIT;
+
+starting permutation: s1_update s1_commit s2_key_share s1_vacuum s2_commit
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s1_commit: COMMIT;
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s1_vacuum: VACUUM FREEZE tab_freeze;
+step s2_commit: COMMIT;
+
+starting permutation: s1_update s1_commit s2_key_share s2_commit s1_vacuum
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s1_commit: COMMIT;
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s2_commit: COMMIT;
+step s1_vacuum: VACUUM FREEZE tab_freeze;
+
+starting permutation: s1_update s2_key_share s1_commit s1_vacuum s2_commit
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s1_commit: COMMIT;
+step s1_vacuum: VACUUM FREEZE tab_freeze;
+step s2_commit: COMMIT;
+
+starting permutation: s1_update s2_key_share s1_commit s2_commit s1_vacuum
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s1_commit: COMMIT;
+step s2_commit: COMMIT;
+step s1_vacuum: VACUUM FREEZE tab_freeze;
+
+starting permutation: s1_update s2_key_share s2_commit s1_commit s1_vacuum
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s2_commit: COMMIT;
+step s1_commit: COMMIT;
+step s1_vacuum: VACUUM FREEZE tab_freeze;
+
+starting permutation: s2_key_share s1_update s1_commit s1_vacuum s2_commit
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s1_commit: COMMIT;
+step s1_vacuum: VACUUM FREEZE tab_freeze;
+step s2_commit: COMMIT;
+
+starting permutation: s2_key_share s1_update s1_commit s2_commit s1_vacuum
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s1_commit: COMMIT;
+step s2_commit: COMMIT;
+step s1_vacuum: VACUUM FREEZE tab_freeze;
+
+starting permutation: s2_key_share s1_update s2_commit s1_commit s1_vacuum
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s2_commit: COMMIT;
+step s1_commit: COMMIT;
+step s1_vacuum: VACUUM FREEZE tab_freeze;
+
+starting permutation: s2_key_share s2_commit s1_update s1_commit s1_vacuum
+step s2_key_share: SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE;
+id             
+
+3              
+step s2_commit: COMMIT;
+step s1_update: UPDATE tab_freeze SET x = x + 1 WHERE id = 3;
+step s1_commit: COMMIT;
+step s1_vacuum: VACUUM FREEZE tab_freeze;

diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 46134c4a77eb7f629a6f922650fa2f0d935da49f..07cff08b337805891996f5dbc690f935ceb01b60 100644 (file)
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -30,5 +30,6 @@ test: lock-committed-keyupdate
 test: update-locked-tuple
 test: propagate-lock-delete
 test: tuplelock-conflict
+test: freeze-the-dead
 test: drop-index-concurrently-1
 test: timeouts

diff --git a/src/test/isolation/specs/freeze-the-dead.spec b/src/test/isolation/specs/freeze-the-dead.spec
new file mode 100644 (file)
index 0000000..3cd9965
--- /dev/null
+++ b/src/test/isolation/specs/freeze-the-dead.spec
@@ -0,0 +1,27 @@
+# Test for interactions of tuple freezing with dead, as well as recently-dead
+# tuples using multixacts via FOR KEY SHARE.
+setup
+{
+  CREATE TABLE tab_freeze (
+    id int PRIMARY KEY,
+    name char(3),
+    x int);
+  INSERT INTO tab_freeze VALUES (1, '111', 0);
+  INSERT INTO tab_freeze VALUES (3, '333', 0);
+}
+
+teardown
+{
+  DROP TABLE tab_freeze;
+}
+
+session "s1"
+setup              { BEGIN; }
+step "s1_update"   { UPDATE tab_freeze SET x = x + 1 WHERE id = 3; }
+step "s1_commit"   { COMMIT; }
+step "s1_vacuum"   { VACUUM FREEZE tab_freeze; }
+
+session "s2"
+setup              { BEGIN; }
+step "s2_key_share"    { SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE; }
+step "s2_commit"   { COMMIT; }