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