Fix for regression caused by heap tuple header changes - Mailing list pgsql-patches

This patch fixes a regression caused by my recent changes to heap
tuple header.  The fix is based on the thought that HEAP_MOVED_IN is
not needed any more as soon as HEAP_XMIN_COMMITTED has been set.  So
in tqual.c and vacuum.c the HEAP_MOVED bits are cleared when
HEAP_XMIN_COMMITTED is set.

Vacuum robustness is enhanced by rearranging ifs, so that we have a
chance to elog(ERROR, ...) before an assertion fails.

A new regression test is included.

Servus
 Manfred
diff -ruN ../base/src/backend/commands/vacuum.c src/backend/commands/vacuum.c
--- ../base/src/backend/commands/vacuum.c    2002-07-15 20:39:19.000000000 +0200
+++ src/backend/commands/vacuum.c    2002-07-16 13:41:59.000000000 +0200
@@ -1534,8 +1534,6 @@

             if (!(tuple.t_data->t_infomask & HEAP_XMIN_COMMITTED))
             {
-                if (HeapTupleHeaderGetXvac(tuple.t_data) != myXID)
-                    elog(ERROR, "Invalid XVAC in tuple header");
                 if (tuple.t_data->t_infomask & HEAP_MOVED_IN)
                     elog(ERROR, "HEAP_MOVED_IN was not expected");

@@ -1546,6 +1544,8 @@
                  */
                 if (tuple.t_data->t_infomask & HEAP_MOVED_OFF)
                 {
+                    if (HeapTupleHeaderGetXvac(tuple.t_data) != myXID)
+                        elog(ERROR, "Invalid XVAC in tuple header");
                     if (keep_tuples == 0)
                         continue;
                     if (chain_tuple_moved)        /* some chains was moved
@@ -2009,7 +2009,7 @@

             /*
              * Mark new tuple as moved_in by vacuum and store vacuum XID
-             * in t_cmin !!!
+             * in t_cid !!!
              */
             newtup.t_data->t_infomask &=
                 ~(HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID | HEAP_MOVED_OFF);
@@ -2035,7 +2035,7 @@

             /*
              * Mark old tuple as moved_off by vacuum and store vacuum XID
-             * in t_cmin !!!
+             * in t_cid !!!
              */
             tuple.t_data->t_infomask &=
                 ~(HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID | HEAP_MOVED_IN);
@@ -2088,12 +2088,12 @@
                 tuple.t_data = (HeapTupleHeader) PageGetItem(page, itemid);
                 if (tuple.t_data->t_infomask & HEAP_XMIN_COMMITTED)
                     continue;
-                if (HeapTupleHeaderGetXvac(tuple.t_data) != myXID)
-                    elog(ERROR, "Invalid XVAC in tuple header (4)");
                 if (tuple.t_data->t_infomask & HEAP_MOVED_IN)
                     elog(ERROR, "HEAP_MOVED_IN was not expected (2)");
                 if (tuple.t_data->t_infomask & HEAP_MOVED_OFF)
                 {
+                    if (HeapTupleHeaderGetXvac(tuple.t_data) != myXID)
+                        elog(ERROR, "Invalid XVAC in tuple header (4)");
                     /* some chains was moved while */
                     if (chain_tuple_moved)
                     {            /* cleaning this page */
@@ -2117,6 +2117,8 @@
                         keep_tuples--;
                     }
                 }
+                else
+                    elog(ERROR, "HEAP_MOVED_OFF was expected (2)");
             }
         }

@@ -2226,17 +2228,18 @@
             tuple.t_data = (HeapTupleHeader) PageGetItem(page, itemid);
             if (!(tuple.t_data->t_infomask & HEAP_XMIN_COMMITTED))
             {
+                if (!(tuple.t_data->t_infomask & HEAP_MOVED))
+                    elog(ERROR, "HEAP_MOVED_OFF/HEAP_MOVED_IN was expected");
                 if (HeapTupleHeaderGetXvac(tuple.t_data) != myXID)
                     elog(ERROR, "Invalid XVAC in tuple header (2)");
                 if (tuple.t_data->t_infomask & HEAP_MOVED_IN)
                 {
                     tuple.t_data->t_infomask |= HEAP_XMIN_COMMITTED;
+                    tuple.t_data->t_infomask &= ~HEAP_MOVED;
                     num_tuples++;
                 }
-                else if (tuple.t_data->t_infomask & HEAP_MOVED_OFF)
-                    tuple.t_data->t_infomask |= HEAP_XMIN_INVALID;
                 else
-                    elog(ERROR, "HEAP_MOVED_OFF/HEAP_MOVED_IN was expected");
+                    tuple.t_data->t_infomask |= HEAP_XMIN_INVALID;
             }
         }
         LockBuffer(buf, BUFFER_LOCK_UNLOCK);
@@ -2305,15 +2308,15 @@

                 if (!(tuple.t_data->t_infomask & HEAP_XMIN_COMMITTED))
                 {
-                    if (HeapTupleHeaderGetXvac(tuple.t_data) != myXID)
-                        elog(ERROR, "Invalid XVAC in tuple header (3)");
                     if (tuple.t_data->t_infomask & HEAP_MOVED_OFF)
                     {
+                        if (HeapTupleHeaderGetXvac(tuple.t_data) != myXID)
+                            elog(ERROR, "Invalid XVAC in tuple header (3)");
                         itemid->lp_flags &= ~LP_USED;
                         num_tuples++;
                     }
                     else
-                        elog(ERROR, "HEAP_MOVED_OFF was expected (2)");
+                        elog(ERROR, "HEAP_MOVED_OFF was expected (3)");
                 }

             }
diff -ruN ../base/src/backend/utils/time/tqual.c src/backend/utils/time/tqual.c
--- ../base/src/backend/utils/time/tqual.c    2002-06-21 02:12:23.000000000 +0200
+++ src/backend/utils/time/tqual.c    2002-07-16 13:19:59.000000000 +0200
@@ -92,7 +92,10 @@
                 if (TransactionIdIsInProgress(HeapTupleHeaderGetXvac(tuple)))
                     return false;
                 if (TransactionIdDidCommit(HeapTupleHeaderGetXvac(tuple)))
+                {
                     tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                    tuple->t_infomask &= ~HEAP_MOVED;
+                }
                 else
                 {
                     tuple->t_infomask |= HEAP_XMIN_INVALID;
@@ -219,6 +222,7 @@
                     return false;
                 }
                 tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                tuple->t_infomask &= ~HEAP_MOVED;
             }
         }
         else if (tuple->t_infomask & HEAP_MOVED_IN)
@@ -228,7 +232,10 @@
                 if (TransactionIdIsInProgress(HeapTupleHeaderGetXvac(tuple)))
                     return false;
                 if (TransactionIdDidCommit(HeapTupleHeaderGetXvac(tuple)))
+                {
                     tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                    tuple->t_infomask &= ~HEAP_MOVED;
+                }
                 else
                 {
                     tuple->t_infomask |= HEAP_XMIN_INVALID;
@@ -336,6 +343,7 @@
                     return false;
                 }
                 tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                tuple->t_infomask &= ~HEAP_MOVED;
             }
         }
         else if (tuple->t_infomask & HEAP_MOVED_IN)
@@ -345,7 +353,10 @@
                 if (TransactionIdIsInProgress(HeapTupleHeaderGetXvac(tuple)))
                     return false;
                 if (TransactionIdDidCommit(HeapTupleHeaderGetXvac(tuple)))
+                {
                     tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                    tuple->t_infomask &= ~HEAP_MOVED;
+                }
                 else
                 {
                     tuple->t_infomask |= HEAP_XMIN_INVALID;
@@ -389,6 +400,7 @@
                     return HeapTupleInvisible;
                 }
                 tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                tuple->t_infomask &= ~HEAP_MOVED;
             }
         }
         else if (tuple->t_infomask & HEAP_MOVED_IN)
@@ -398,7 +410,10 @@
                 if (TransactionIdIsInProgress(HeapTupleHeaderGetXvac(tuple)))
                     return HeapTupleInvisible;
                 if (TransactionIdDidCommit(HeapTupleHeaderGetXvac(tuple)))
+                {
                     tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                    tuple->t_infomask &= ~HEAP_MOVED;
+                }
                 else
                 {
                     tuple->t_infomask |= HEAP_XMIN_INVALID;
@@ -520,6 +535,7 @@
                     return false;
                 }
                 tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                tuple->t_infomask &= ~HEAP_MOVED;
             }
         }
         else if (tuple->t_infomask & HEAP_MOVED_IN)
@@ -529,7 +545,10 @@
                 if (TransactionIdIsInProgress(HeapTupleHeaderGetXvac(tuple)))
                     return false;
                 if (TransactionIdDidCommit(HeapTupleHeaderGetXvac(tuple)))
+                {
                     tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                    tuple->t_infomask &= ~HEAP_MOVED;
+                }
                 else
                 {
                     tuple->t_infomask |= HEAP_XMIN_INVALID;
@@ -651,6 +670,7 @@
                     return false;
                 }
                 tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                tuple->t_infomask &= ~HEAP_MOVED;
             }
         }
         else if (tuple->t_infomask & HEAP_MOVED_IN)
@@ -660,7 +680,10 @@
                 if (TransactionIdIsInProgress(HeapTupleHeaderGetXvac(tuple)))
                     return false;
                 if (TransactionIdDidCommit(HeapTupleHeaderGetXvac(tuple)))
+                {
                     tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                    tuple->t_infomask &= ~HEAP_MOVED;
+                }
                 else
                 {
                     tuple->t_infomask |= HEAP_XMIN_INVALID;
@@ -809,6 +832,7 @@
                 return HEAPTUPLE_DEAD;
             }
             tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+            tuple->t_infomask &= ~HEAP_MOVED;
         }
         else if (tuple->t_infomask & HEAP_MOVED_IN)
         {
@@ -817,7 +841,10 @@
             if (TransactionIdIsInProgress(HeapTupleHeaderGetXvac(tuple)))
                 return HEAPTUPLE_INSERT_IN_PROGRESS;
             if (TransactionIdDidCommit(HeapTupleHeaderGetXvac(tuple)))
+            {
                 tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+                tuple->t_infomask &= ~HEAP_MOVED;
+            }
             else
             {
                 tuple->t_infomask |= HEAP_XMIN_INVALID;
diff -ruN ../base/src/test/regress/expected/vacuum.out src/test/regress/expected/vacuum.out
--- ../base/src/test/regress/expected/vacuum.out    2002-07-16 14:12:56.000000000 +0200
+++ src/test/regress/expected/vacuum.out    2002-07-16 14:26:05.000000000 +0200
@@ -0,0 +1,59 @@
+--
+-- VACUUM
+--
+CREATE TABLE vactst (i INT);
+INSERT INTO vactst VALUES (1);
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst VALUES (0);
+SELECT count(*) FROM vactst;
+ count
+-------
+  2049
+(1 row)
+
+DELETE FROM vactst WHERE i != 0;
+SELECT * FROM vactst;
+ i
+---
+ 0
+(1 row)
+
+VACUUM FULL vactst;
+UPDATE vactst SET i = i + 1;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst VALUES (0);
+SELECT count(*) FROM vactst;
+ count
+-------
+  2049
+(1 row)
+
+DELETE FROM vactst WHERE i != 0;
+VACUUM FULL vactst;
+DELETE FROM vactst;
+SELECT * FROM vactst;
+ i
+---
+(0 rows)
+
+DROP TABLE vactst;
diff -ruN ../base/src/test/regress/parallel_schedule src/test/regress/parallel_schedule
--- ../base/src/test/regress/parallel_schedule    2002-07-10 23:29:32.000000000 +0200
+++ src/test/regress/parallel_schedule    2002-07-16 14:01:23.000000000 +0200
@@ -38,7 +38,7 @@
 # ----------
 # The third group of parallel test
 # ----------
-test: constraints triggers create_misc create_aggregate create_operator create_index inherit
+test: constraints triggers create_misc create_aggregate create_operator create_index inherit vacuum

 # Depends on the above
 test: create_view
diff -ruN ../base/src/test/regress/serial_schedule src/test/regress/serial_schedule
--- ../base/src/test/regress/serial_schedule    2002-07-10 23:31:16.000000000 +0200
+++ src/test/regress/serial_schedule    2002-07-16 14:01:45.000000000 +0200
@@ -50,6 +50,7 @@
 test: create_operator
 test: create_index
 test: inherit
+test: vacuum
 test: create_view
 test: sanity_check
 test: errors
diff -ruN ../base/src/test/regress/sql/vacuum.sql src/test/regress/sql/vacuum.sql
--- ../base/src/test/regress/sql/vacuum.sql    2002-07-16 14:36:30.000000000 +0200
+++ src/test/regress/sql/vacuum.sql    2002-07-16 14:22:43.000000000 +0200
@@ -0,0 +1,42 @@
+--
+-- VACUUM
+--
+
+CREATE TABLE vactst (i INT);
+INSERT INTO vactst VALUES (1);
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst VALUES (0);
+SELECT count(*) FROM vactst;
+DELETE FROM vactst WHERE i != 0;
+SELECT * FROM vactst;
+VACUUM FULL vactst;
+UPDATE vactst SET i = i + 1;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst SELECT * FROM vactst;
+INSERT INTO vactst VALUES (0);
+SELECT count(*) FROM vactst;
+DELETE FROM vactst WHERE i != 0;
+VACUUM FULL vactst;
+DELETE FROM vactst;
+SELECT * FROM vactst;
+
+DROP TABLE vactst;