Fix a longstanding bug in VACUUM FULL's handling of update chains. The code

did not expect that a DEAD tuple could follow a RECENTLY_DEAD tuple in an
update chain, but because the OldestXmin rule for determining deadness is a
simplification of reality, it is possible for this situation to occur
(implying that the RECENTLY_DEAD tuple is in fact dead to all observers,
but this patch does not attempt to exploit that).  The code would follow a
chain forward all the way, but then stop before a DEAD tuple when backing
up, meaning that not all of the chain got moved.  This could lead to copying
the chain multiple times (resulting in duplicate copies of the live tuple at
its end), or leaving dangling index entries behind (which, aside from
generating warnings from later vacuums, creates a risk of wrong query
results or bogus duplicate-key errors once the heap slot the index entry
points to is repopulated).

The fix is to recheck HeapTupleSatisfiesVacuum while following a chain
forward, and to stop if a DEAD tuple is reached.  Each contiguous group
of RECENTLY_DEAD tuples will therefore be copied as a separate chain.
The patch also adds a couple of extra sanity checks to verify correct
behavior.

Per report and test case from Pavan Deolasee.

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 26053bb418bf8c624b61ae4221d86beb92e1ef54..d3b832ad4aa43ee67cc3f6702a8ed7b0cb65a789 100644 (file)
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -13,7 +13,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.263.2.2 2005/08/25 22:07:16 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.263.2.3 2007/03/14 18:49:26 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1650,6 +1650,15 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                         * we cannot find the parent tuple in vtlinks.  This may be
                         * overly conservative; AFAICS it would be safe to move the
                         * chain.
+                        *
+                        * Also, because we distinguish DEAD and RECENTLY_DEAD tuples
+                        * using OldestXmin, which is a rather coarse test, it is quite
+                        * possible to have an update chain in which a tuple we think is
+                        * RECENTLY_DEAD links forward to one that is definitely DEAD.
+                        * In such a case the RECENTLY_DEAD tuple must actually be dead,
+                        * but it seems too complicated to try to make VACUUM remove it.
+                        * We treat each contiguous set of RECENTLY_DEAD tuples as a
+                        * separately movable chain, ignoring any intervening DEAD ones.
                         */
                        if (((tuple.t_data->t_infomask & HEAP_UPDATED) &&
                         !TransactionIdPrecedes(HeapTupleHeaderGetXmin(tuple.t_data),
@@ -1662,6 +1671,7 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                Buffer          Cbuf = buf;
                                bool            freeCbuf = false;
                                bool            chain_move_failed = false;
+                               bool            moved_target = false;
                                ItemPointerData Ctid;
                                HeapTupleData tp = tuple;
                                Size            tlen = tuple_len;
@@ -1689,7 +1699,14 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                 * If this tuple is in the begin/middle of the chain then
                                 * we have to move to the end of chain.  As with any
                                 * t_ctid chase, we have to verify that each new tuple
-                                * is really the descendant of the tuple we came from.
+                                * is really the descendant of the tuple we came from;
+                                * however, here we need even more than the normal amount of
+                                * paranoia.  If t_ctid links forward to a tuple determined
+                                * to be DEAD, then depending on where that tuple is, it
+                                * might already have been removed, and perhaps even replaced
+                                * by a MOVED_IN tuple.  We don't want to include any DEAD
+                                * tuples in the chain, so we have to recheck
+                                * HeapTupleSatisfiesVacuum.
                                 */
                                while (!(tp.t_data->t_infomask & (HEAP_XMAX_INVALID |
                                                                                          HEAP_MARKED_FOR_UPDATE)) &&
@@ -1703,6 +1720,8 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                        OffsetNumber nextOffnum;
                                        ItemId          nextItemid;
                                        HeapTupleHeader nextTdata;
+                                       uint16          sv_infomask;
+                                       HTSV_Result     nextTstatus;
 
                                        nextTid = tp.t_data->t_ctid;
                                        priorXmax = HeapTupleHeaderGetXmax(tp.t_data);
@@ -1733,6 +1752,22 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                                ReleaseBuffer(nextBuf);
                                                break;
                                        }
+                                       /* must check for DEAD or MOVED_IN tuple, too */
+                                       sv_infomask = nextTdata->t_infomask;
+                                       nextTstatus = HeapTupleSatisfiesVacuum(nextTdata,
+                                                                                                                  OldestXmin);
+                                       /* not sure hint-bit change can happen, but be careful */
+                                       if (sv_infomask != nextTdata->t_infomask)
+                                               SetBufferCommitInfoNeedsSave(nextBuf);
+                                       if (nextTstatus == HEAPTUPLE_DEAD ||
+                                               nextTstatus == HEAPTUPLE_INSERT_IN_PROGRESS)
+                                       {
+                                               ReleaseBuffer(nextBuf);
+                                               break;
+                                       }
+                                       /* if it's MOVED_OFF we shoulda moved this one with it */
+                                       if (nextTstatus == HEAPTUPLE_DELETE_IN_PROGRESS)
+                                               elog(ERROR, "updated tuple is already HEAP_MOVED_OFF");
                                        /* OK, switch our attention to the next tuple in chain */
                                        tp.t_datamcxt = NULL;
                                        tp.t_data = nextTdata;
@@ -1803,6 +1838,11 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                        free_vtmove--;
                                        num_vtmove++;
 
+                                       /* Remember if we reached the original target tuple */
+                                       if (ItemPointerGetBlockNumber(&tp.t_self) == blkno &&
+                                               ItemPointerGetOffsetNumber(&tp.t_self) == offnum)
+                                               moved_target = true;
+
                                        /* At beginning of chain? */
                                        if (!(tp.t_data->t_infomask & HEAP_UPDATED) ||
                                                TransactionIdPrecedes(HeapTupleHeaderGetXmin(tp.t_data),
@@ -1872,6 +1912,13 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                        ReleaseBuffer(Cbuf);
                                freeCbuf = false;
 
+                               /* Double-check that we will move the current target tuple */
+                               if (!moved_target && !chain_move_failed)
+                               {
+                                       elog(DEBUG2, "failed to chain back to target --- cannot continue repair_frag");
+                                       chain_move_failed = true;
+                               }
+
                                if (chain_move_failed)
                                {
                                        /*