I wrote:
> Just thinking about this ... I wonder why we need to call
> TransactionIdIsInProgress() at all rather than believing the answer from
> the snapshot? Under what circumstances could TransactionIdIsInProgress()
> return true where XidInMVCCSnapshot() had not?
I experimented with the attached patch, which replaces
HeapTupleSatisfiesMVCC's calls of TransactionIdIsInProgress with
XidInMVCCSnapshot, and then as a cross-check has all the "return false"
exits from XidInMVCCSnapshot assert !TransactionIdIsInProgress().
The asserts did not fire in the standard regression tests nor in a
pgbench run, which is surely not proof of anything but it suggests
that I'm not totally nuts.
I wouldn't commit the changes in XidInMVCCSnapshot for real, but
otherwise this is a possibly committable patch.
regards, tom lane
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index de7b3fc..c8f59f7 100644
*** a/src/backend/utils/time/tqual.c
--- b/src/backend/utils/time/tqual.c
***************
*** 10,21 ****
* the passed-in buffer. The caller must hold not only a pin, but at least
* shared buffer content lock on the buffer containing the tuple.
*
! * NOTE: must check TransactionIdIsInProgress (which looks in PGXACT array)
* before TransactionIdDidCommit/TransactionIdDidAbort (which look in
* pg_clog). Otherwise we have a race condition: we might decide that a
* just-committed transaction crashed, because none of the tests succeed.
* xact.c is careful to record commit/abort in pg_clog before it unsets
! * MyPgXact->xid in PGXACT array. That fixes that problem, but it also
* means there is a window where TransactionIdIsInProgress and
* TransactionIdDidCommit will both return true. If we check only
* TransactionIdDidCommit, we could consider a tuple committed when a
--- 10,22 ----
* the passed-in buffer. The caller must hold not only a pin, but at least
* shared buffer content lock on the buffer containing the tuple.
*
! * NOTE: When using a non-MVCC snapshot, we must check
! * TransactionIdIsInProgress (which looks in the PGXACT array)
* before TransactionIdDidCommit/TransactionIdDidAbort (which look in
* pg_clog). Otherwise we have a race condition: we might decide that a
* just-committed transaction crashed, because none of the tests succeed.
* xact.c is careful to record commit/abort in pg_clog before it unsets
! * MyPgXact->xid in the PGXACT array. That fixes that problem, but it also
* means there is a window where TransactionIdIsInProgress and
* TransactionIdDidCommit will both return true. If we check only
* TransactionIdDidCommit, we could consider a tuple committed when a
***************
*** 26,31 ****
--- 27,37 ----
* subtransactions of our own main transaction and so there can't be any
* race condition.
*
+ * When using an MVCC snapshot, we rely on XidInMVCCSnapshot rather than
+ * TransactionIdIsInProgress, but the logic is otherwise the same: do not
+ * check pg_clog until after deciding that the xact is no longer in progress.
+ *
+ *
* Summary of visibility functions:
*
* HeapTupleSatisfiesMVCC()
*************** HeapTupleSatisfiesMVCC(HeapTuple htup, S
*** 961,967 ****
if (TransactionIdIsCurrentTransactionId(xvac))
return false;
! if (!TransactionIdIsInProgress(xvac))
{
if (TransactionIdDidCommit(xvac))
{
--- 967,973 ----
if (TransactionIdIsCurrentTransactionId(xvac))
return false;
! if (!XidInMVCCSnapshot(xvac, snapshot))
{
if (TransactionIdDidCommit(xvac))
{
*************** HeapTupleSatisfiesMVCC(HeapTuple htup, S
*** 980,986 ****
if (!TransactionIdIsCurrentTransactionId(xvac))
{
! if (TransactionIdIsInProgress(xvac))
return false;
if (TransactionIdDidCommit(xvac))
SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
--- 986,992 ----
if (!TransactionIdIsCurrentTransactionId(xvac))
{
! if (XidInMVCCSnapshot(xvac, snapshot))
return false;
if (TransactionIdDidCommit(xvac))
SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
*************** HeapTupleSatisfiesMVCC(HeapTuple htup, S
*** 1035,1041 ****
else
return false; /* deleted before scan started */
}
! else if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmin(tuple)))
return false;
else if (TransactionIdDidCommit(HeapTupleHeaderGetRawXmin(tuple)))
SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
--- 1041,1047 ----
else
return false; /* deleted before scan started */
}
! else if (XidInMVCCSnapshot(HeapTupleHeaderGetRawXmin(tuple), snapshot))
return false;
else if (TransactionIdDidCommit(HeapTupleHeaderGetRawXmin(tuple)))
SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
*************** HeapTupleSatisfiesMVCC(HeapTuple htup, S
*** 1048,1061 ****
return false;
}
}
! /*
! * By here, the inserting transaction has committed - have to check
! * when...
! */
! if (!HeapTupleHeaderXminFrozen(tuple)
! && XidInMVCCSnapshot(HeapTupleHeaderGetRawXmin(tuple), snapshot))
! return false; /* treat as still in progress */
if (tuple->t_infomask & HEAP_XMAX_INVALID) /* xid invalid or aborted */
return true;
--- 1054,1068 ----
return false;
}
}
+ else
+ {
+ /* xmin is committed, but maybe not according to our snapshot */
+ if (!HeapTupleHeaderXminFrozen(tuple) &&
+ XidInMVCCSnapshot(HeapTupleHeaderGetRawXmin(tuple), snapshot))
+ return false; /* treat as still in progress */
+ }
! /* by here, the inserting transaction has committed */
if (tuple->t_infomask & HEAP_XMAX_INVALID) /* xid invalid or aborted */
return true;
*************** HeapTupleSatisfiesMVCC(HeapTuple htup, S
*** 1082,1096 ****
else
return false; /* deleted before scan started */
}
! if (TransactionIdIsInProgress(xmax))
return true;
if (TransactionIdDidCommit(xmax))
! {
! /* updating transaction committed, but when? */
! if (XidInMVCCSnapshot(xmax, snapshot))
! return true; /* treat as still in progress */
! return false;
! }
/* it must have aborted or crashed */
return true;
}
--- 1089,1098 ----
else
return false; /* deleted before scan started */
}
! if (XidInMVCCSnapshot(xmax, snapshot))
return true;
if (TransactionIdDidCommit(xmax))
! return false; /* updating transaction committed */
/* it must have aborted or crashed */
return true;
}
*************** HeapTupleSatisfiesMVCC(HeapTuple htup, S
*** 1105,1111 ****
return false; /* deleted before scan started */
}
! if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
return true;
if (!TransactionIdDidCommit(HeapTupleHeaderGetRawXmax(tuple)))
--- 1107,1113 ----
return false; /* deleted before scan started */
}
! if (XidInMVCCSnapshot(HeapTupleHeaderGetRawXmax(tuple), snapshot))
return true;
if (!TransactionIdDidCommit(HeapTupleHeaderGetRawXmax(tuple)))
*************** HeapTupleSatisfiesMVCC(HeapTuple htup, S
*** 1120,1131 ****
SetHintBits(tuple, buffer, HEAP_XMAX_COMMITTED,
HeapTupleHeaderGetRawXmax(tuple));
}
! /*
! * OK, the deleting transaction committed too ... but when?
! */
! if (XidInMVCCSnapshot(HeapTupleHeaderGetRawXmax(tuple), snapshot))
! return true; /* treat as still in progress */
return false;
}
--- 1122,1135 ----
SetHintBits(tuple, buffer, HEAP_XMAX_COMMITTED,
HeapTupleHeaderGetRawXmax(tuple));
}
+ else
+ {
+ /* xmax is committed, but maybe not according to our snapshot */
+ if (XidInMVCCSnapshot(HeapTupleHeaderGetRawXmax(tuple), snapshot))
+ return true; /* treat as still in progress */
+ }
! /* xmax transaction committed */
return false;
}
*************** XidInMVCCSnapshot(TransactionId xid, Sna
*** 1461,1467 ****
--- 1465,1474 ----
/* Any xid < xmin is not in-progress */
if (TransactionIdPrecedes(xid, snapshot->xmin))
+ {
+ Assert(!TransactionIdIsInProgress(xid));
return false;
+ }
/* Any xid >= xmax is in-progress */
if (TransactionIdFollowsOrEquals(xid, snapshot->xmax))
return true;
*************** XidInMVCCSnapshot(TransactionId xid, Sna
*** 1503,1509 ****
--- 1510,1519 ----
* xmax.
*/
if (TransactionIdPrecedes(xid, snapshot->xmin))
+ {
+ Assert(!TransactionIdIsInProgress(xid));
return false;
+ }
}
for (i = 0; i < snapshot->xcnt; i++)
*************** XidInMVCCSnapshot(TransactionId xid, Sna
*** 1534,1540 ****
--- 1544,1553 ----
* xmax.
*/
if (TransactionIdPrecedes(xid, snapshot->xmin))
+ {
+ Assert(!TransactionIdIsInProgress(xid));
return false;
+ }
}
/*
*************** XidInMVCCSnapshot(TransactionId xid, Sna
*** 1549,1554 ****
--- 1562,1568 ----
}
}
+ Assert(!TransactionIdIsInProgress(xid));
return false;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
