Fix for regression caused by heap tuple header changes

Started by Manfred Koizarabout 24 years ago3 messagespatches

mkoi-pg@aon.at

about 24 years ago

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

Bruce Momjian

bruce@momjian.us

about 24 years ago

In reply to: Manfred Koizar (#1)

Re: Fix for regression caused by heap tuple header changes

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Manfred Koizar wrote:

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;

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian

bruce@momjian.us

about 24 years ago

In reply to: Manfred Koizar (#1)

Re: Fix for regression caused by heap tuple header changes

Patch applied. Thanks.

---------------------------------------------------------------------------

Manfred Koizar wrote:

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.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026