Re: Fix for regression caused by heap tuple header changes

From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Manfred Koizar <mkoi-pg(at)aon(dot)at>
Cc: pgsql-patches(at)postgresql(dot)org
Subject: Re: Fix for regression caused by heap tuple header changes
Date: 2002-07-18 20:36:56
Message-ID: 200207182036.g6IKauK14987@candle.pha.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-patches


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(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

In response to

Browse pgsql-patches by date

  From Date Subject
Next Message Tom Lane 2002-07-18 20:40:18 Re: Between Node
Previous Message Bruce Momjian 2002-07-18 20:36:24 Re: minor GEQO fixes