More heap tuple header fixes

Started by Manfred Koizarabout 24 years ago7 messagespatches

Jump to latest

Manfred Koizar

mkoi-pg@aon.at

about 24 years ago

This patch fixes one serious bug (runaway INSERT) and a few rare (and
hard to reproduce) error conditions.

Servus
Manfred

Tom Lane

tgl@sss.pgh.pa.us

about 24 years ago

In reply to: Manfred Koizar (#1)

Re: More heap tuple header fixes

Manfred Koizar <mkoi-pg@aon.at> writes:

+	tup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
HeapTupleHeaderSetXmin(tup->t_data, GetCurrentTransactionId());
HeapTupleHeaderSetCmin(tup->t_data, cid);
HeapTupleHeaderSetXmaxInvalid(tup->t_data);
-	HeapTupleHeaderSetCmax(tup->t_data, FirstCommandId);
-	tup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
+	/*
+	 * Do *not* set Cmax!  This would overwrite Cmin.
+	 */
+	/* HeapTupleHeaderSetCmax(tup->t_data, FirstCommandId); */
tup->t_data->t_infomask |= HEAP_XMAX_INVALID;

This sort of thing crystallizes the vague unease I had about those
HeapTupleHeader macros.

I'd recommend redesigning the HeapTupleHeaderSet macros so that they
do not do any setting of t_infomask bits, or even take any conditional
action based on them, but solely Assert() that the bits are already
in the appropriate state to allow storing of the value to be stored.
Then, all uses have to be checked to ensure that t_infomask is coerced
into the right state *before* doing HeapTupleHeaderSetFoo. Anything
else is subject to order-of-operations mistakes that were not errors
before, and cannot be detected by the macros as now defined. The
cmax-set-is-not-okay bug illustrated above is a perfect example of
what I'm talking about.

regards, tom lane

Manfred Koizar

mkoi-pg@aon.at

about 24 years ago

In reply to: Tom Lane (#2)

Re: More heap tuple header fixes

On Sat, 20 Jul 2002 16:27:14 -0400, Tom Lane <tgl@sss.pgh.pa.us>
wrote:

I'd recommend redesigning the HeapTupleHeaderSet macros so that they
do not do any setting of t_infomask bits, or even take any conditional
action based on them,

The HEAP_XMIN_IS_XMAX bit is exclusively managed by these macros.
Pulling the handling of this bit out of the macros and spreading it to
the places, where the macros are used, cannot make the whole thing
more robust. This would mean, the caller had to decide whether to
store Cmax into t_cid or t_xmax...

but solely Assert() that the bits are already
in the appropriate state to allow storing of the value to be stored.
Then, all uses have to be checked to ensure that t_infomask is coerced
into the right state *before* doing HeapTupleHeaderSetFoo.

Apart from HEAP_XMIN_IS_XMAX this was my intention; we already do
this with HEAP_MOVED. I could add an assertion to SetCmax:
Assert(!((tup)->t_infomask & HEAP_XMAX_INVALID));

OTOH I thought about putting *more* logic into the macros to make
their use less fragile. For example SetXmaxInvalid could set the
HEAP_XMAX_INVALID bit, likewise SetCminInvalid with XMIN_INVALID.

Anyway, with this patch applied the heap tuple header changes should
be able to survive the next two weeks. I don't want to hack together
a quick change now, before I go on vacation. Let's find the perfect
solution, when I'm back ...

Servus
Manfred

Tom Lane

tgl@sss.pgh.pa.us

about 24 years ago

In reply to: Manfred Koizar (#3)

Re: More heap tuple header fixes

Manfred Koizar <mkoi-pg@aon.at> writes:

On Sat, 20 Jul 2002 16:27:14 -0400, Tom Lane <tgl@sss.pgh.pa.us>
wrote:

I'd recommend redesigning the HeapTupleHeaderSet macros so that they
do not do any setting of t_infomask bits, or even take any conditional
action based on them,

The HEAP_XMIN_IS_XMAX bit is exclusively managed by these macros.
Pulling the handling of this bit out of the macros and spreading it to
the places, where the macros are used, cannot make the whole thing
more robust. This would mean, the caller had to decide whether to
store Cmax into t_cid or t_xmax...

But the present scheme is very obviously not robust.

Anyway, with this patch applied the heap tuple header changes should
be able to survive the next two weeks. I don't want to hack together
a quick change now, before I go on vacation. Let's find the perfect
solution, when I'm back ...

I'm on the road too. But I want to see a different solution before
we release. I do not trust the current code at all.

regards, tom lane

Bruce Momjian

bruce@momjian.us

about 24 years ago

In reply to: Manfred Koizar (#3)

Re: More heap tuple header fixes

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:

On Sat, 20 Jul 2002 16:27:14 -0400, Tom Lane <tgl@sss.pgh.pa.us>
wrote:

I'd recommend redesigning the HeapTupleHeaderSet macros so that they
do not do any setting of t_infomask bits, or even take any conditional
action based on them,

The HEAP_XMIN_IS_XMAX bit is exclusively managed by these macros.
Pulling the handling of this bit out of the macros and spreading it to
the places, where the macros are used, cannot make the whole thing
more robust. This would mean, the caller had to decide whether to
store Cmax into t_cid or t_xmax...

but solely Assert() that the bits are already
in the appropriate state to allow storing of the value to be stored.
Then, all uses have to be checked to ensure that t_infomask is coerced
into the right state *before* doing HeapTupleHeaderSetFoo.

Apart from HEAP_XMIN_IS_XMAX this was my intention; we already do
this with HEAP_MOVED. I could add an assertion to SetCmax:
Assert(!((tup)->t_infomask & HEAP_XMAX_INVALID));

OTOH I thought about putting *more* logic into the macros to make
their use less fragile. For example SetXmaxInvalid could set the
HEAP_XMAX_INVALID bit, likewise SetCminInvalid with XMIN_INVALID.

Anyway, with this patch applied the heap tuple header changes should
be able to survive the next two weeks. I don't want to hack together
a quick change now, before I go on vacation. Let's find the perfect
solution, when I'm back ...

Servus
Manfred

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

-- 
  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: More heap tuple header fixes

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 one serious bug (runaway INSERT) and a few rare (and
hard to reproduce) error conditions.

Servus
Manfred

diff -ruN ../base/src/backend/access/heap/heapam.c src/backend/access/heap/heapam.c
--- ../base/src/backend/access/heap/heapam.c	2002-07-20 17:27:18.000000000 +0200
+++ src/backend/access/heap/heapam.c	2002-07-20 19:43:19.000000000 +0200
@@ -1123,11 +1123,14 @@
CheckMaxObjectId(HeapTupleGetOid(tup));
}

+	tup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
HeapTupleHeaderSetXmin(tup->t_data, GetCurrentTransactionId());
HeapTupleHeaderSetCmin(tup->t_data, cid);
HeapTupleHeaderSetXmaxInvalid(tup->t_data);
-	HeapTupleHeaderSetCmax(tup->t_data, FirstCommandId);
-	tup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
+	/*
+	 * Do *not* set Cmax!  This would overwrite Cmin.
+	 */
+	/* HeapTupleHeaderSetCmax(tup->t_data, FirstCommandId); */
tup->t_data->t_infomask |= HEAP_XMAX_INVALID;
tup->t_tableOid = relation->rd_id;

@@ -2147,7 +2150,11 @@

if (redo)
{
-		htup->t_infomask &= ~(HEAP_XMAX_COMMITTED |
+		/*
+		 * On redo from WAL we cannot rely on a tqual-routine
+		 * to have reset HEAP_MOVED.
+		 */
+		htup->t_infomask &= ~(HEAP_MOVED | HEAP_XMAX_COMMITTED |
HEAP_XMAX_INVALID | HEAP_MARKED_FOR_UPDATE);
HeapTupleHeaderSetXmax(htup, record->xl_xid);
HeapTupleHeaderSetCmax(htup, FirstCommandId);
@@ -2320,7 +2327,11 @@
}
else
{
-			htup->t_infomask &= ~(HEAP_XMAX_COMMITTED |
+			/*
+			 * On redo from WAL we cannot rely on a tqual-routine
+			 * to have reset HEAP_MOVED.
+			 */
+			htup->t_infomask &= ~(HEAP_MOVED | HEAP_XMAX_COMMITTED |
HEAP_XMAX_INVALID | HEAP_MARKED_FOR_UPDATE);
HeapTupleHeaderSetXmax(htup, record->xl_xid);
HeapTupleHeaderSetCmax(htup, FirstCommandId);
diff -ruN ../base/src/backend/utils/time/tqual.c src/backend/utils/time/tqual.c
--- ../base/src/backend/utils/time/tqual.c	2002-07-20 17:27:19.000000000 +0200
+++ src/backend/utils/time/tqual.c	2002-07-20 19:27:03.000000000 +0200
@@ -83,6 +83,7 @@
return false;
}
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+				tuple->t_infomask &= ~HEAP_MOVED;
}
}
else if (tuple->t_infomask & HEAP_MOVED_IN)

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  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

almost 24 years ago

In reply to: Manfred Koizar (#1)

Re: More heap tuple header fixes

Patch applied. Thanks.

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

Manfred Koizar wrote:

This patch fixes one serious bug (runaway INSERT) and a few rare (and
hard to reproduce) error conditions.

Servus
Manfred

diff -ruN ../base/src/backend/access/heap/heapam.c src/backend/access/heap/heapam.c
--- ../base/src/backend/access/heap/heapam.c	2002-07-20 17:27:18.000000000 +0200
+++ src/backend/access/heap/heapam.c	2002-07-20 19:43:19.000000000 +0200
@@ -1123,11 +1123,14 @@
CheckMaxObjectId(HeapTupleGetOid(tup));
}

+	tup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
HeapTupleHeaderSetXmin(tup->t_data, GetCurrentTransactionId());
HeapTupleHeaderSetCmin(tup->t_data, cid);
HeapTupleHeaderSetXmaxInvalid(tup->t_data);
-	HeapTupleHeaderSetCmax(tup->t_data, FirstCommandId);
-	tup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
+	/*
+	 * Do *not* set Cmax!  This would overwrite Cmin.
+	 */
+	/* HeapTupleHeaderSetCmax(tup->t_data, FirstCommandId); */
tup->t_data->t_infomask |= HEAP_XMAX_INVALID;
tup->t_tableOid = relation->rd_id;

@@ -2147,7 +2150,11 @@

if (redo)
{
-		htup->t_infomask &= ~(HEAP_XMAX_COMMITTED |
+		/*
+		 * On redo from WAL we cannot rely on a tqual-routine
+		 * to have reset HEAP_MOVED.
+		 */
+		htup->t_infomask &= ~(HEAP_MOVED | HEAP_XMAX_COMMITTED |
HEAP_XMAX_INVALID | HEAP_MARKED_FOR_UPDATE);
HeapTupleHeaderSetXmax(htup, record->xl_xid);
HeapTupleHeaderSetCmax(htup, FirstCommandId);
@@ -2320,7 +2327,11 @@
}
else
{
-			htup->t_infomask &= ~(HEAP_XMAX_COMMITTED |
+			/*
+			 * On redo from WAL we cannot rely on a tqual-routine
+			 * to have reset HEAP_MOVED.
+			 */
+			htup->t_infomask &= ~(HEAP_MOVED | HEAP_XMAX_COMMITTED |
HEAP_XMAX_INVALID | HEAP_MARKED_FOR_UPDATE);
HeapTupleHeaderSetXmax(htup, record->xl_xid);
HeapTupleHeaderSetCmax(htup, FirstCommandId);
diff -ruN ../base/src/backend/utils/time/tqual.c src/backend/utils/time/tqual.c
--- ../base/src/backend/utils/time/tqual.c	2002-07-20 17:27:19.000000000 +0200
+++ src/backend/utils/time/tqual.c	2002-07-20 19:27:03.000000000 +0200
@@ -83,6 +83,7 @@
return false;
}
tuple->t_infomask |= HEAP_XMIN_COMMITTED;
+				tuple->t_infomask &= ~HEAP_MOVED;
}
}
else if (tuple->t_infomask & HEAP_MOVED_IN)

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  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