On Mon, Feb 16, 2015 at 4:11 PM, Peter Geoghegan <p...@heroku.com> wrote:
>>> Jim Nasby said something about setting the HEAP_XMIN_INVALID hint bit.
>>> Maybe he is right...if that can be made to be reliable (always
>>> WAL-logged), it could be marginally better than setting xmin to
>>> invalidTransactionId.
>>
>>
>> I'm not a big fan of that. The xmin-invalid bit is currently always just a
>> hint.
>
> Well, Andres makes the point that that isn't quite so.
Hmm. So the tqual.c routines actually check "if
(HeapTupleHeaderXminInvalid(tuple))". Which is:
#define HeapTupleHeaderXminInvalid(tup) \
( \
((tup)->t_infomask & (HEAP_XMIN_COMMITTED|HEAP_XMIN_INVALID)) == \
HEAP_XMIN_INVALID \
)
What appears to happen if I try to only use the HEAP_XMIN_INVALID bit
(and not explicitly set xmin to InvalidTransactionId, and not
explicitly check that xmin isn't InvalidTransactionId in each
HeapTupleSatisfies* routine) is that after a little while, Jeff Janes'
tool shows up spurious unique violations, as if some tuple has become
visible when it shouldn't have. I guess this is because the
HEAP_XMIN_COMMITTED hint bit can still be set, which in effect
invalidates the HEAP_XMIN_INVALID hint bit.
It takes about 2 minutes before this happens for the first time when
fsync = off, following a fresh initdb, which is unacceptable. I'm not
sure if it's worth trying to figure out how HEAP_XMIN_COMMITTED gets
set. Not that I'm 100% sure that that's what this really is; it just
seems very likely.
Attached broken patch (broken_visible.patch) shows the changes made to
reveal this problem. Only the changes to tqual.c and heap_delete()
should matter here, since I did not test recovery.
I also thought about unifying the check for "if
(TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
InvalidTransactionId))" with the conventional
HeapTupleHeaderXminInvalid() macro, and leaving everything else as-is.
This is no good either, and for similar reasons - control often won't
reach the macro, which is behind a check of "if
(!HeapTupleHeaderXminCommitted(tuple))".
The best I think we can hope for is having a dedicated "if
(TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
InvalidTransactionId))" macro HeapTupleHeaderSuperDeleted() to do the
work each time, which does not need to be checked so often. A second
attached patch (compact_tqual_works.patch - which is non-broken,
AFAICT) shows how this is possible, while also moving the check
further down within each tqual.c routine (which seems more in keeping
with the fact that super deletion is a relatively obscure concept). I
haven't been able to break this variant using my existing test suite,
so this seems promising as a way of reducing tqual.c clutter. However,
as I said the other day, this is basically just polish.
--
Peter Geoghegan
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 0aa3e57..b777c56 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2899,7 +2899,7 @@ l1:
}
else
{
- HeapTupleHeaderSetXmin(tp.t_data, InvalidTransactionId);
+ HeapTupleHeaderSetXminInvalid(tp.t_data);
}
/* Make sure there is no forward chain link in t_ctid */
@@ -7382,12 +7382,12 @@ heap_xlog_delete(XLogReaderState *record)
htup->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
htup->t_infomask2 &= ~HEAP_KEYS_UPDATED;
HeapTupleHeaderClearHotUpdated(htup);
+ if (xlrec->flags & XLOG_HEAP_KILLED_SPECULATIVE_TUPLE)
+ HeapTupleHeaderSetXminInvalid(htup);
+
fix_infomask_from_infobits(xlrec->infobits_set,
&htup->t_infomask, &htup->t_infomask2);
- if (!(xlrec->flags & XLOG_HEAP_KILLED_SPECULATIVE_TUPLE))
- HeapTupleHeaderSetXmax(htup, xlrec->xmax);
- else
- HeapTupleHeaderSetXmin(htup, InvalidTransactionId);
+ HeapTupleHeaderSetXmax(htup, xlrec->xmax);
HeapTupleHeaderSetCmax(htup, FirstCommandId, false);
/* Mark the page as a candidate for pruning */
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 99bb417..fd857b1 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -170,13 +170,6 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
- /*
- * Never return "super-deleted" tuples
- */
- if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
- InvalidTransactionId))
- return false;
-
if (!HeapTupleHeaderXminCommitted(tuple))
{
if (HeapTupleHeaderXminInvalid(tuple))
@@ -735,15 +728,6 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
snapshot->xmin = snapshot->xmax = InvalidTransactionId;
snapshot->speculativeToken = 0;
- /*
- * Never return "super-deleted" tuples
- *
- * XXX: Comment this code out and you'll get conflicts within
- * ExecLockUpdateTuple(), which result in an infinite loop.
- */
- if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
- InvalidTransactionId))
- return false;
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -960,13 +944,6 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
- /*
- * Never return "super-deleted" tuples
- */
- if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
- InvalidTransactionId))
- return false;
-
if (!HeapTupleHeaderXminCommitted(tuple))
{
if (HeapTupleHeaderXminInvalid(tuple))
@@ -1171,13 +1148,6 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
Assert(htup->t_tableOid != InvalidOid);
/*
- * Immediately VACUUM "super-deleted" tuples
- */
- if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
- InvalidTransactionId))
- return HEAPTUPLE_DEAD;
-
- /*
* Has inserting transaction committed?
*
* If the inserting transaction aborted, then the tuple was never visible
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 99bb417..aed0eeb 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -170,13 +170,6 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
- /*
- * Never return "super-deleted" tuples
- */
- if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
- InvalidTransactionId))
- return false;
-
if (!HeapTupleHeaderXminCommitted(tuple))
{
if (HeapTupleHeaderXminInvalid(tuple))
@@ -269,6 +262,9 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
}
}
+ if (HeapTupleHeaderSuperDeleted(tuple))
+ return false;
+
/* by here, the inserting transaction has committed */
if (tuple->t_infomask & HEAP_XMAX_INVALID) /* xid invalid or aborted */
@@ -735,16 +731,6 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
snapshot->xmin = snapshot->xmax = InvalidTransactionId;
snapshot->speculativeToken = 0;
- /*
- * Never return "super-deleted" tuples
- *
- * XXX: Comment this code out and you'll get conflicts within
- * ExecLockUpdateTuple(), which result in an infinite loop.
- */
- if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
- InvalidTransactionId))
- return false;
-
if (!HeapTupleHeaderXminCommitted(tuple))
{
if (HeapTupleHeaderXminInvalid(tuple))
@@ -861,6 +847,9 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
}
}
+ if (HeapTupleHeaderSuperDeleted(tuple))
+ return false;
+
/* by here, the inserting transaction has committed */
if (tuple->t_infomask & HEAP_XMAX_INVALID) /* xid invalid or aborted */
@@ -960,13 +949,6 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
- /*
- * Never return "super-deleted" tuples
- */
- if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
- InvalidTransactionId))
- return false;
-
if (!HeapTupleHeaderXminCommitted(tuple))
{
if (HeapTupleHeaderXminInvalid(tuple))
@@ -1067,6 +1049,9 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
}
}
+ if (HeapTupleHeaderSuperDeleted(tuple))
+ return false;
+
/*
* By here, the inserting transaction has committed - have to check
* when...
@@ -1171,13 +1156,6 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
Assert(htup->t_tableOid != InvalidOid);
/*
- * Immediately VACUUM "super-deleted" tuples
- */
- if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
- InvalidTransactionId))
- return HEAPTUPLE_DEAD;
-
- /*
* Has inserting transaction committed?
*
* If the inserting transaction aborted, then the tuple was never visible
@@ -1270,6 +1248,9 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
*/
}
+ if (HeapTupleHeaderSuperDeleted(tuple))
+ return HEAPTUPLE_DEAD;
+
/*
* Okay, the inserter committed, so it was good at some point. Now what
* about the deleting transaction?
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index d2ad910..5906df4 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -305,6 +305,15 @@ struct HeapTupleHeaderData
)
/*
+ * Was tuple "super deleted" following unsuccessful speculative insertion (i.e.
+ * conflict was detected at insertion time)?
+ */
+#define HeapTupleHeaderSuperDeleted(tup) \
+( \
+ (!TransactionIdIsValid(HeapTupleHeaderGetRawXmin(tup))) \
+)
+
+/*
* HeapTupleHeaderGetRawXmax gets you the raw Xmax field. To find out the Xid
* that updated a tuple, you might need to resolve the MultiXactId if certain
* bits are set. HeapTupleHeaderGetUpdateXid checks those bits and takes care
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
