Triggered assertion "!(tp.t_data->t_infomask & HEAP_XMAX_INVALID)" in heap_delete() on HEAD [PATCH]
Hi
In the process of re-verifying my serializable lock consistency patch, I ran
the fk_concurrency testsuite against *unpatched* HEAD for comparison.
My build of HEAD had asserts enabled, and I promptly triggered
Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID))
in heap_delete().
The seems wrong, if result was set to HeapTupleUpdated because the tuple was invisible
to the crosscheck snapshot, its xmax may very well be invalid.
Simply removing the assert isn't an option, because right after the assert the tuple's
xmax is copied into update_xmax. Thus the attached patch takes care to set update_xmax
to InvalidTransactionId explicitly in case the update is prevented by the crosscheck snapshot.
heap_update() suffers from the same problem and is treated similarly by the patch.
Note that this patch conflicts with the serializable_lock_consistency patch, since it
changes that assert too, but in a different way.
best regards,
Florian Pflug
Attachments:
fix_assert_xmaxinvalid.v1.patchapplication/octet-stream; name=fix_assert_xmaxinvalid.v1.patchDownload
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4020906..e074213 100644
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
*************** l1:
*** 2175,2191 ****
{
/* Perform additional check for transaction-snapshot mode RI updates */
if (!HeapTupleSatisfiesVisibility(&tp, crosscheck, buffer))
result = HeapTupleUpdated;
}
if (result != HeapTupleMayBeUpdated)
{
Assert(result == HeapTupleSelfUpdated ||
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
- Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID));
*ctid = tp.t_data->t_ctid;
- *update_xmax = HeapTupleHeaderGetXmax(tp.t_data);
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(tp.t_self), ExclusiveLock);
--- 2175,2198 ----
{
/* Perform additional check for transaction-snapshot mode RI updates */
if (!HeapTupleSatisfiesVisibility(&tp, crosscheck, buffer))
+ {
result = HeapTupleUpdated;
+ *update_xmax = InvalidTransactionId;
+ }
}
+ else if (result != HeapTupleMayBeUpdated)
+ {
+ Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID));
+ *update_xmax = HeapTupleHeaderGetXmax(tp.t_data);
+ }
+
if (result != HeapTupleMayBeUpdated)
{
Assert(result == HeapTupleSelfUpdated ||
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
*ctid = tp.t_data->t_ctid;
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(tp.t_self), ExclusiveLock);
*************** l2:
*** 2527,2533 ****
--- 2534,2548 ----
{
/* Perform additional check for transaction-snapshot mode RI updates */
if (!HeapTupleSatisfiesVisibility(&oldtup, crosscheck, buffer))
+ {
result = HeapTupleUpdated;
+ *update_xmax = InvalidTransactionId;
+ }
+ }
+ else if (result != HeapTupleMayBeUpdated)
+ {
+ Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID));
+ *update_xmax = HeapTupleHeaderGetXmax(oldtup.t_data);
}
if (result != HeapTupleMayBeUpdated)
*************** l2:
*** 2535,2543 ****
Assert(result == HeapTupleSelfUpdated ||
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
- Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID));
*ctid = oldtup.t_data->t_ctid;
- *update_xmax = HeapTupleHeaderGetXmax(oldtup.t_data);
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(oldtup.t_self), ExclusiveLock);
--- 2550,2556 ----
Florian Pflug <fgp@phlo.org> writes:
In the process of re-verifying my serializable lock consistency patch, I ran
the fk_concurrency testsuite against *unpatched* HEAD for comparison.
My build of HEAD had asserts enabled, and I promptly triggered
Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID))
in heap_delete().
The seems wrong, if result was set to HeapTupleUpdated because the tuple was invisible
to the crosscheck snapshot, its xmax may very well be invalid.
This patch seems certainly wrong. Please provide an actual test case
rather than just asserting we should change this.
regards, tom lane
On Dec14, 2010, at 21:18 , Tom Lane wrote:
Florian Pflug <fgp@phlo.org> writes:
In the process of re-verifying my serializable lock consistency patch, I ran
the fk_concurrency testsuite against *unpatched* HEAD for comparison.My build of HEAD had asserts enabled, and I promptly triggered
Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID))
in heap_delete().The seems wrong, if result was set to HeapTupleUpdated because the tuple was invisible
to the crosscheck snapshot, its xmax may very well be invalid.This patch seems certainly wrong. Please provide an actual test case
rather than just asserting we should change this.
Running my FK concurrency test suite against HEAD as of today with 100 transaction / client triggers this within a few seconds or so. The test suite can be found at https://github.com/fgp/fk_concurrency.
./fk_concurrency.sh <tx/client> native <path to pg> <host or patch to socket>
Could you explain what seems to be wrong with my patch? If you believe that it's impossible for a tuple to be visible under the query's snapshot but invisible to the crosscheck snapshot, unless it was deleted, that's *not* the case! For RI checks in serializable transactions, the *crosscheck* snapshot is the serializable snapshot, while the query's snapshot is obtained with GetLatetsSnapshot(). This is the relevant snippet from ri_trigger.c, ri_PerformCheck():
if (IsolationUsesXactSnapshot() && detectNewRows)
{
CommandCounterIncrement(); /* be sure all my own work is visible */
test_snapshot = GetLatestSnapshot();
crosscheck_snapshot = GetTransactionSnapshot();
}
else
{
/* the default SPI behavior is okay */
test_snapshot = InvalidSnapshot;
crosscheck_snapshot = InvalidSnapshot;
}
best regards,
Florian Pflug
Florian Pflug <fgp@phlo.org> writes:
Could you explain what seems to be wrong with my patch?
I'm unconvinced that this is the proper response to whatever the problem
is; and if it is the right response, it seems to still need a good bit
more work. You didn't even update the functions' header comments, let
alone look at their callers to see how they'd be affected by an
InvalidTransactionId result.
regards, tom lane
On Dec14, 2010, at 21:52 , Tom Lane wrote:
Florian Pflug <fgp@phlo.org> writes:
Could you explain what seems to be wrong with my patch?
I'm unconvinced that this is the proper response to whatever the problem
is;
Well, you didn't comment on the part of my previous e-mail that *did*
explain why I believe this is the proper response...
and if it is the right response, it seems to still need a good bit
more work. You didn't even update the functions' header comments, let
alone look at their callers to see how they'd be affected by an
InvalidTransactionId result.
Well, I hit this while re-verifying the serializable lock consistency stuff
with a current HEAD, so I didn't really want to spend more time on this than
necessary. Especially since that patch replaces the assert in question anyway.
So I moved the assert to a safe place and HeapTupleHeaderGetXmax() with
it, since if the assert fails HeapTupleHeaderGetXmax() will return garbage
anyway (read: a multi-xid instead of an xid in some cases!).
For non-assert-enabled builds, the only effect of the patch is thus to
consistently return InvalidTransactionId if the crosscheck snapshot turns
HeapTupleMayBeUpdated into HeapTupleUpdated. Which certainly seems to be
an improvement over sometimes returning InvalidTransactionId, sometimes
a locker's xid, and sometime's a multi-xid.
best regards,
Florian Pflug
On Dec14, 2010, at 22:34 , Florian Pflug wrote:
For non-assert-enabled builds, the only effect of the patch is thus to
consistently return InvalidTransactionId if the crosscheck snapshot turns
HeapTupleMayBeUpdated into HeapTupleUpdated. Which certainly seems to be
an improvement over sometimes returning InvalidTransactionId, sometimes
a locker's xid, and sometime's a multi-xid.
I've updated the patch to explain that in the comments above heap_update()
and heap_delete(). I've taken a brief look at the callers of these functions,
but fail to see how to improve things there. Things work currently because
crosschecking is only used in serializable mode, while the tuple's xmax is
only required in read committed mode to double-check the tuple chain when
following the ctid pointers. But the API doesn't enforce that at all :-(
I'm not willing to clean that mess up, since the serializable lock consistency
patch changes all these areas, *and* makes the cleanup easier by getting rid
of the crosscheck snapshot entirely.
I still believe that applying this patch now is worth it, for the reasons
already explained, but in the end that's obviously something for a committer
to decide.
Patch with updated comments attached.
best regards,
Florian Pflug
Attachments:
fix_assert_xmaxinvalid.v2.patchapplication/octet-stream; name=fix_assert_xmaxinvalid.v2.patch; x-unix-mode=0644Download
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4020906..2dca6a9 100644
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
*************** simple_heap_insert(Relation relation, He
*** 2041,2050 ****
* HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
* (the last only possible if wait == false).
*
! * In the failure cases, the routine returns the tuple's t_ctid and t_xmax.
! * If t_ctid is the same as tid, the tuple was deleted; if different, the
! * tuple was updated, and t_ctid is the location of the replacement tuple.
! * (t_xmax is needed to verify that the replacement tuple matches.)
*/
HTSU_Result
heap_delete(Relation relation, ItemPointer tid,
--- 2041,2055 ----
* HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
* (the last only possible if wait == false).
*
! * In case of a failure, the reason can be determined from the tuple's
! * t_ctid and t_xmax values returned in ctid and update_xmax.
! * If ctid is the same as otid and update_xmax is InvalidTransactionId the
! * tupe was neither updated nor deleted, but is invisible to the crosscheck
! * snapshot. If ctid equals otid and update_xmax is a valid transactionid,
! * the tuple was deleted by the transaction in update_xmax.
! * Otherwise, if ctid differes from otid, the tuple was updated and ctid is
! * the location of the replacement tuple; update_xmax should in this case be
! * used to verify that the replacement tuple matches.
*/
HTSU_Result
heap_delete(Relation relation, ItemPointer tid,
*************** l1:
*** 2175,2191 ****
{
/* Perform additional check for transaction-snapshot mode RI updates */
if (!HeapTupleSatisfiesVisibility(&tp, crosscheck, buffer))
result = HeapTupleUpdated;
}
if (result != HeapTupleMayBeUpdated)
{
Assert(result == HeapTupleSelfUpdated ||
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
- Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID));
*ctid = tp.t_data->t_ctid;
- *update_xmax = HeapTupleHeaderGetXmax(tp.t_data);
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(tp.t_self), ExclusiveLock);
--- 2180,2203 ----
{
/* Perform additional check for transaction-snapshot mode RI updates */
if (!HeapTupleSatisfiesVisibility(&tp, crosscheck, buffer))
+ {
result = HeapTupleUpdated;
+ *update_xmax = InvalidTransactionId;
+ }
+ }
+ else if (result != HeapTupleMayBeUpdated)
+ {
+ Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID));
+ *update_xmax = HeapTupleHeaderGetXmax(tp.t_data);
}
+
if (result != HeapTupleMayBeUpdated)
{
Assert(result == HeapTupleSelfUpdated ||
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
*ctid = tp.t_data->t_ctid;
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(tp.t_self), ExclusiveLock);
*************** simple_heap_delete(Relation relation, It
*** 2363,2372 ****
* update was done. However, any TOAST changes in the new tuple's
* data are not reflected into *newtup.
*
! * In the failure cases, the routine returns the tuple's t_ctid and t_xmax.
! * If t_ctid is the same as otid, the tuple was deleted; if different, the
! * tuple was updated, and t_ctid is the location of the replacement tuple.
! * (t_xmax is needed to verify that the replacement tuple matches.)
*/
HTSU_Result
heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
--- 2375,2389 ----
* update was done. However, any TOAST changes in the new tuple's
* data are not reflected into *newtup.
*
! * In case of a failure, the reason can be determined from the tuple's
! * t_ctid and t_xmax values returned in ctid and update_xmax.
! * If ctid is the same as otid and update_xmax is InvalidTransactionId the
! * tupe was neither updated nor deleted, but is invisible to the crosscheck
! * snapshot. If ctid equals otid and update_xmax is a valid transactionid,
! * the tuple was deleted by the transaction in update_xmax.
! * Otherwise, if ctid differes from otid, the tuple was updated and ctid is
! * the location of the replacement tuple; update_xmax should in this case be
! * used to verify that the replacement tuple matches.
*/
HTSU_Result
heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
*************** l2:
*** 2527,2533 ****
--- 2544,2558 ----
{
/* Perform additional check for transaction-snapshot mode RI updates */
if (!HeapTupleSatisfiesVisibility(&oldtup, crosscheck, buffer))
+ {
result = HeapTupleUpdated;
+ *update_xmax = InvalidTransactionId;
+ }
+ }
+ else if (result != HeapTupleMayBeUpdated)
+ {
+ Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID));
+ *update_xmax = HeapTupleHeaderGetXmax(oldtup.t_data);
}
if (result != HeapTupleMayBeUpdated)
*************** l2:
*** 2535,2543 ****
Assert(result == HeapTupleSelfUpdated ||
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
- Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID));
*ctid = oldtup.t_data->t_ctid;
- *update_xmax = HeapTupleHeaderGetXmax(oldtup.t_data);
UnlockReleaseBuffer(buffer);
if (have_tuple_lock)
UnlockTuple(relation, &(oldtup.t_self), ExclusiveLock);
--- 2560,2566 ----