Patch VACUUM problem with moving chain of update tuples when source

and destination of a tuple lie on the same page.
(Previously fixed in REL7_0 branch, now apply to current.)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c7496c6c46c531652a27dad872dc176db93fb70e..9a790aae2e3195310238926947cb0e83d7c6d0d7 100644 (file)
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.168 2000/10/16 17:08:05 momjian Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.169 2000/10/22 19:49:43 tgl Exp $
  *
 
  *-------------------------------------------------------------------------
@@ -669,6 +669,7 @@ scan_heap(VRelStats *vacrelstats, Relation onerel,
                                                                                           tuple.t_data->t_cmin))
                                        {
                                                tuple.t_data->t_infomask |= HEAP_XMIN_INVALID;
+                                               pgchanged = true;
                                                tupgone = true;
                                        }
                                        else
@@ -683,6 +684,7 @@ scan_heap(VRelStats *vacrelstats, Relation onerel,
                                                                                                tuple.t_data->t_cmin))
                                        {
                                                tuple.t_data->t_infomask |= HEAP_XMIN_INVALID;
+                                               pgchanged = true;
                                                tupgone = true;
                                        }
                                        else
@@ -730,8 +732,10 @@ scan_heap(VRelStats *vacrelstats, Relation onerel,
                                {
                                        if (tuple.t_data->t_infomask & HEAP_MARKED_FOR_UPDATE)
                                        {
-                                               pgchanged = true;
                                                tuple.t_data->t_infomask |= HEAP_XMAX_INVALID;
+                                               tuple.t_data->t_infomask &=
+                                                       ~(HEAP_XMAX_COMMITTED | HEAP_MARKED_FOR_UPDATE);
+                                               pgchanged = true;
                                        }
                                        else
                                                tupgone = true;
@@ -746,6 +750,8 @@ scan_heap(VRelStats *vacrelstats, Relation onerel,
                                        if (tuple.t_data->t_infomask & HEAP_MARKED_FOR_UPDATE)
                                        {
                                                tuple.t_data->t_infomask |= HEAP_XMAX_INVALID;
+                                               tuple.t_data->t_infomask &=
+                                                       ~(HEAP_XMAX_COMMITTED | HEAP_MARKED_FOR_UPDATE);
                                                pgchanged = true;
                                        }
                                        else
@@ -759,6 +765,8 @@ scan_heap(VRelStats *vacrelstats, Relation onerel,
                                         * from crashed process. - vadim 06/02/97
                                         */
                                        tuple.t_data->t_infomask |= HEAP_XMAX_INVALID;
+                                       tuple.t_data->t_infomask &=
+                                               ~(HEAP_XMAX_COMMITTED | HEAP_MARKED_FOR_UPDATE);
                                        pgchanged = true;
                                }
                                else
@@ -818,6 +826,14 @@ scan_heap(VRelStats *vacrelstats, Relation onerel,
                        {
                                ItemId          lpp;
 
+                               /*
+                                * Here we are building a temporary copy of the page with
+                                * dead tuples removed.  Below we will apply
+                                * PageRepairFragmentation to the copy, so that we can
+                                * determine how much space will be available after
+                                * removal of dead tuples.  But note we are NOT changing
+                                * the real page yet...
+                                */
                                if (tempPage == (Page) NULL)
                                {
                                        Size            pageSize;
@@ -827,14 +843,12 @@ scan_heap(VRelStats *vacrelstats, Relation onerel,
                                        memmove(tempPage, page, pageSize);
                                }
 
+                               /* mark it unused on the temp page */
                                lpp = &(((PageHeader) tempPage)->pd_linp[offnum - 1]);
-
-                               /* mark it unused */
                                lpp->lp_flags &= ~LP_USED;
 
                                vacpage->offsets[vacpage->offsets_free++] = offnum;
                                tups_vacuumed++;
-
                        }
                        else
                        {
@@ -1397,20 +1411,45 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                        tuple.t_datamcxt = NULL;
                                        tuple.t_data = (HeapTupleHeader) PageGetItem(Cpage, Citemid);
                                        tuple_len = tuple.t_len = ItemIdGetLength(Citemid);
-                                       /* Get page to move in */
+
+                                       /*
+                                        * make a copy of the source tuple, and then mark the
+                                        * source tuple MOVED_OFF.
+                                        */
+                                       heap_copytuple_with_tuple(&tuple, &newtup);
+
+                                       RelationInvalidateHeapTuple(onerel, &tuple);
+
+                                       TransactionIdStore(myXID, (TransactionId *) &(tuple.t_data->t_cmin));
+                                       tuple.t_data->t_infomask &=
+                                               ~(HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID | HEAP_MOVED_IN);
+                                       tuple.t_data->t_infomask |= HEAP_MOVED_OFF;
+
+                                       /* Get page to move to */
                                        cur_buffer = ReadBuffer(onerel, destvacpage->blkno);
 
                                        /*
                                         * We should LockBuffer(cur_buffer) but don't, at the
-                                        * moment. If you'll do LockBuffer then UNLOCK it
-                                        * before index_insert: unique btree-s call heap_fetch
-                                        * to get t_infomask of inserted heap tuple !!!
+                                        * moment.  This should be safe enough, since we have
+                                        * exclusive lock on the whole relation.
+                                        * If you'll do LockBuffer then UNLOCK it before
+                                        * index_insert: unique btree-s call heap_fetch to get
+                                        * t_infomask of inserted heap tuple !!!
                                         */
                                        ToPage = BufferGetPage(cur_buffer);
 
                                        /*
                                         * If this page was not used before - clean it.
                                         *
+                                        * NOTE: a nasty bug used to lurk here.  It is possible
+                                        * for the source and destination pages to be the same
+                                        * (since this tuple-chain member can be on a page lower
+                                        * than the one we're currently processing in the outer
+                                        * loop).  If that's true, then after vacuum_page() the
+                                        * source tuple will have been moved, and tuple.t_data
+                                        * will be pointing at garbage.  Therefore we must do
+                                        * everything that uses tuple.t_data BEFORE this step!!
+                                        *
                                         * This path is different from the other callers of
                                         * vacuum_page, because we have already incremented the
                                         * vacpage's offsets_used field to account for the
@@ -1428,8 +1467,11 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                                vacuum_page(ToPage, destvacpage);
                                                destvacpage->offsets_used = sv_offsets_used;
                                        }
-                                       heap_copytuple_with_tuple(&tuple, &newtup);
-                                       RelationInvalidateHeapTuple(onerel, &tuple);
+
+                                       /*
+                                        * Update the state of the copied tuple, and store it
+                                        * on the destination page.
+                                        */
                                        TransactionIdStore(myXID, (TransactionId *) &(newtup.t_data->t_cmin));
                                        newtup.t_data->t_infomask &=
                                                ~(HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID | HEAP_MOVED_OFF);
@@ -1450,8 +1492,8 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                                last_move_dest_block = destvacpage->blkno;
 
                                        /*
-                                        * Set t_ctid pointing to itself for last tuple in
-                                        * chain and to next tuple in chain otherwise.
+                                        * Set new tuple's t_ctid pointing to itself for last
+                                        * tuple in chain, and to next tuple in chain otherwise.
                                         */
                                        if (!ItemPointerIsValid(&Ctid))
                                                newtup.t_data->t_ctid = newtup.t_self;
@@ -1459,11 +1501,6 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                                newtup.t_data->t_ctid = Ctid;
                                        Ctid = newtup.t_self;
 
-                                       TransactionIdStore(myXID, (TransactionId *) &(tuple.t_data->t_cmin));
-                                       tuple.t_data->t_infomask &=
-                                               ~(HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID | HEAP_MOVED_IN);
-                                       tuple.t_data->t_infomask |= HEAP_MOVED_OFF;
-
                                        num_moved++;
 
                                        /*
@@ -1504,10 +1541,7 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
                                                }
                                        }
                                        WriteBuffer(cur_buffer);
-                                       if (Cbuf == buf)
-                                               ReleaseBuffer(Cbuf);
-                                       else
-                                               WriteBuffer(Cbuf);
+                                       WriteBuffer(Cbuf);
                                }
                                cur_buffer = InvalidBuffer;
                                pfree(vtmove);