Re: Make HeapTupleSatisfiesMVCC more concurrent - Mailing list pgsql-hackers

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