Re: Fix for regression caused by heap tuple header changes - Mailing list 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@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