Re: Improvement of procArray.xmin for VACUUM - Mailing list pgsql-patches

This is an optimization so I was thinking of something simpler, like
incrementing/decrementing a counter every time we allocate/free a
snapshot (like in the patch), and updating MyProc->xmin only if there
are no open snapshots.  I don't think it is worth tracking anything more
complicated than that.  Is that now possible to do cleanly?  I am having
trouble getting this to work in the attached patch.

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Simon Riggs wrote:
> >> How much does it cost to optimise for this case?
>
> > Zero cost.  It involves just tracking if there are any old snapshots,
>
> I will be fascinated to hear how you define that as zero cost.  It might
> be relatively low, but it's not zero, and for many simple cases (eg,
> all single-statement transactions) the benefit will indeed be zero.
> We need some serious consideration of the costs and benefits, not airy
> dismissals.
>
> I had originally thought that we could avoid CopySnapshot copying,
> which might buy back more than enough to cover the cost of tracking
> reference counts ... but in a quick look yesterday it seemed that the
> high-use code paths would still need a copy, because they are always
> copying off the static variables filled by GetTransactionSnapshot.
> Changing that would come with a tremendous memory penalty, or else
> a time penalty to scan the ProcArray twice to see how big the arrays
> need to be.
>
> [ thinks a bit... ]  The other way we might possibly tackle that is
> to avoid copying by the expedient of just using those static snapshot
> variables in-place.  SerializableSnapshot surely need never be copied
> since it remains unchanged till end of xact, and no use of the snap
> will survive longer than that.  In simple cases LatestSnapshot is not
> overwritten until the prior value is no longer needed, either.  If
> we could arrange to copy it only when setting up a cursor, or other
> long-lived usage, we'd be ahead of the game.  But I'd certainly want
> a management layer in there to ensure that we don't overwrite a value
> that *is* still in use, and offhand I'm not sure what that layer would
> need to look like.  Possibly some sort of double indirection so that
> callers would have a Snapshot pointer that remained valid after we'd
> copied off arrays that need to be updated?  We already do have double
> indirection in the form of the Snapshot and xip[] pointers ... maybe
> attach refcounts to the xip arrays not the Snapshots per se?
>
>             regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: 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  <bruce@momjian.us>          http://momjian.us
  EnterpriseDB                               http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: src/backend/catalog/index.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/catalog/index.c,v
retrieving revision 1.281
diff -c -c -r1.281 index.c
*** src/backend/catalog/index.c    25 Mar 2007 19:45:14 -0000    1.281
--- src/backend/catalog/index.c    26 Mar 2007 21:06:30 -0000
***************
*** 1358,1364 ****
      ExprContext *econtext;
      Snapshot    snapshot;
      TransactionId OldestXmin;
!
      /*
       * sanity checks
       */
--- 1358,1364 ----
      ExprContext *econtext;
      Snapshot    snapshot;
      TransactionId OldestXmin;
!
      /*
       * sanity checks
       */
***************
*** 1555,1561 ****
      ExecDropSingleTupleTableSlot(slot);

      FreeExecutorState(estate);
!
      /* These may have been pointing to the now-gone estate */
      indexInfo->ii_ExpressionsState = NIL;
      indexInfo->ii_PredicateState = NIL;
--- 1555,1563 ----
      ExecDropSingleTupleTableSlot(slot);

      FreeExecutorState(estate);
!     if (snapshot != SnapshotNow)
!         FreeSnapshot(snapshot);
!
      /* These may have been pointing to the now-gone estate */
      indexInfo->ii_ExpressionsState = NIL;
      indexInfo->ii_PredicateState = NIL;
Index: src/backend/commands/cluster.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/cluster.c,v
retrieving revision 1.157
diff -c -c -r1.157 cluster.c
*** src/backend/commands/cluster.c    13 Mar 2007 00:33:39 -0000    1.157
--- src/backend/commands/cluster.c    26 Mar 2007 21:06:30 -0000
***************
*** 204,209 ****
--- 204,210 ----
              /* Start a new transaction for each relation. */
              StartTransactionCommand();
              /* functions in indexes may want a snapshot set */
+             FreeSnapshot(ActiveSnapshot);
              ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
              cluster_rel(rvtc, true);
              CommitTransactionCommand();
Index: src/backend/commands/indexcmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v
retrieving revision 1.157
diff -c -c -r1.157 indexcmds.c
*** src/backend/commands/indexcmds.c    13 Mar 2007 00:33:39 -0000    1.157
--- src/backend/commands/indexcmds.c    26 Mar 2007 21:06:31 -0000
***************
*** 493,500 ****
       * We also set ActiveSnapshot to this snap, since functions in indexes may
       * need a snapshot.
       */
!     snapshot = CopySnapshot(GetTransactionSnapshot());
!     ActiveSnapshot = snapshot;

      /*
       * Scan the index and the heap, insert any missing index entries.
--- 493,500 ----
       * We also set ActiveSnapshot to this snap, since functions in indexes may
       * need a snapshot.
       */
!     FreeSnapshot(ActiveSnapshot);
!     ActiveSnapshot = snapshot = CopySnapshot(GetTransactionSnapshot());

      /*
       * Scan the index and the heap, insert any missing index entries.
***************
*** 1304,1309 ****
--- 1304,1310 ----

          StartTransactionCommand();
          /* functions in indexes may want a snapshot set */
+         FreeSnapshot(ActiveSnapshot);
          ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
          if (reindex_relation(relid, true))
              ereport(NOTICE,
Index: src/backend/commands/trigger.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/trigger.c,v
retrieving revision 1.214
diff -c -c -r1.214 trigger.c
*** src/backend/commands/trigger.c    19 Mar 2007 23:38:29 -0000    1.214
--- src/backend/commands/trigger.c    26 Mar 2007 21:06:32 -0000
***************
*** 2645,2651 ****
--- 2645,2654 ----
       */
      events = &afterTriggers->events;
      if (events->head != NULL)
+     {
+         FreeSnapshot(ActiveSnapshot);
          ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
+     }

      /*
       * Run all the remaining triggers.    Loop until they are all gone, just in
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.349
diff -c -c -r1.349 vacuum.c
*** src/backend/commands/vacuum.c    14 Mar 2007 18:48:55 -0000    1.349
--- src/backend/commands/vacuum.c    26 Mar 2007 21:06:33 -0000
***************
*** 412,417 ****
--- 412,418 ----
                  {
                      StartTransactionCommand();
                      /* functions in indexes may want a snapshot set */
+                     FreeSnapshot(ActiveSnapshot);
                      ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
                  }
                  else
***************
*** 470,475 ****
--- 471,477 ----
           * necessary if we are called from autovacuum because autovacuum might
           * continue on to do an ANALYZE-only call.
           */
+         FreeSnapshot(ActiveSnapshot);
          ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
      }

***************
*** 964,969 ****
--- 966,972 ----
      if (vacstmt->full)
      {
          /* functions in indexes may want a snapshot set */
+         FreeSnapshot(ActiveSnapshot);
          ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
      }
      else
Index: src/backend/executor/functions.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/functions.c,v
retrieving revision 1.112
diff -c -c -r1.112 functions.c
*** src/backend/executor/functions.c    13 Mar 2007 00:33:40 -0000    1.112
--- src/backend/executor/functions.c    26 Mar 2007 21:06:33 -0000
***************
*** 315,321 ****
                                          fcache->paramLI);

      /* We assume we don't need to set up ActiveSnapshot for ExecutorStart */
!
      /* Utility commands don't need Executor. */
      if (es->qd->operation != CMD_UTILITY)
      {
--- 315,321 ----
                                          fcache->paramLI);

      /* We assume we don't need to set up ActiveSnapshot for ExecutorStart */
!
      /* Utility commands don't need Executor. */
      if (es->qd->operation != CMD_UTILITY)
      {
Index: src/backend/postmaster/autovacuum.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/postmaster/autovacuum.c,v
retrieving revision 1.38
diff -c -c -r1.38 autovacuum.c
*** src/backend/postmaster/autovacuum.c    23 Mar 2007 21:57:10 -0000    1.38
--- src/backend/postmaster/autovacuum.c    26 Mar 2007 21:06:34 -0000
***************
*** 884,889 ****
--- 884,890 ----
      StartTransactionCommand();

      /* functions in indexes may want a snapshot set */
+     FreeSnapshot(ActiveSnapshot);
      ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());

      /*
Index: src/backend/storage/ipc/procarray.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/storage/ipc/procarray.c,v
retrieving revision 1.23
diff -c -c -r1.23 procarray.c
*** src/backend/storage/ipc/procarray.c    25 Mar 2007 19:45:14 -0000    1.23
--- src/backend/storage/ipc/procarray.c    26 Mar 2007 21:06:35 -0000
***************
*** 498,504 ****
   *----------
   */
  Snapshot
! GetSnapshotData(Snapshot snapshot, bool serializable)
  {
      ProcArrayStruct *arrayP = procArray;
      TransactionId xmin;
--- 498,504 ----
   *----------
   */
  Snapshot
! GetSnapshotData(Snapshot snapshot)
  {
      ProcArrayStruct *arrayP = procArray;
      TransactionId xmin;
***************
*** 510,519 ****

      Assert(snapshot != NULL);

!     /* Serializable snapshot must be computed before any other... */
!     Assert(serializable ?
!            !TransactionIdIsValid(MyProc->xmin) :
!            TransactionIdIsValid(MyProc->xmin));

      /*
       * Allocating space for maxProcs xids is usually overkill; numProcs would
--- 510,517 ----

      Assert(snapshot != NULL);

!     /* Either we set an xmin, or we already have one. */
!     Assert(TransactionIdIsValid(MyProc->xmin) || GetOpenSnapshotCount() == 0);

      /*
       * Allocating space for maxProcs xids is usually overkill; numProcs would
***************
*** 656,664 ****
          }
      }

!     if (serializable)
!         MyProc->xmin = TransactionXmin = xmin;

      LWLockRelease(ProcArrayLock);

      /*
--- 654,681 ----
          }
      }

!     /* DEBUG */
!     if (!TransactionIdIsValid(MyProc->xmin) || GetOpenSnapshotCount() == 0)
!         fprintf(stderr, "%x:%x: set new xmin old=%x new=%x\n", MyProcPid, MyProc->xid, MyProc->xmin, xmin);
!     else
!         fprintf(stderr, "%x:%x: count=%d, not set\n", MyProcPid, MyProc->xid, GetOpenSnapshotCount());

+     /*
+      * ---
+      * We need to set MyProc->xmin in a few cases:
+      *    o  set at transaction start for SERIALIZABLE visibility
+      *    o  set for new command in READ COMMITTED mode so
+      *       VACUUM knows we don't care about completed transactions
+      *
+      *    The problem is that we might have old cursors/portals
+      *    that need to have older transaction visibility, so we
+      *    check to see we have no open Snapshots before making
+      *    the change.
+      * ---
+      */
+     if (!TransactionIdIsValid(MyProc->xmin) || GetOpenSnapshotCount() == 0)
+         MyProc->xmin = TransactionXmin = xmin;
+
      LWLockRelease(ProcArrayLock);

      /*
Index: src/backend/tcop/fastpath.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/tcop/fastpath.c,v
retrieving revision 1.96
diff -c -c -r1.96 fastpath.c
*** src/backend/tcop/fastpath.c    3 Mar 2007 19:32:54 -0000    1.96
--- src/backend/tcop/fastpath.c    26 Mar 2007 21:06:35 -0000
***************
*** 308,313 ****
--- 308,314 ----
       * Now that we know we are in a valid transaction, set snapshot in case
       * needed by function itself or one of the datatype I/O routines.
       */
+     FreeSnapshot(ActiveSnapshot);
      ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());

      /*
Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/tcop/postgres.c,v
retrieving revision 1.529
diff -c -c -r1.529 postgres.c
*** src/backend/tcop/postgres.c    22 Mar 2007 19:55:04 -0000    1.529
--- src/backend/tcop/postgres.c    26 Mar 2007 21:06:36 -0000
***************
*** 738,743 ****
--- 738,744 ----
          {
              if (needSnapshot)
              {
+                 FreeSnapshot(ActiveSnapshot);
                  ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
                  needSnapshot = false;
              }
Index: src/backend/tcop/pquery.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/tcop/pquery.c,v
retrieving revision 1.115
diff -c -c -r1.115 pquery.c
*** src/backend/tcop/pquery.c    13 Mar 2007 00:33:42 -0000    1.115
--- src/backend/tcop/pquery.c    26 Mar 2007 21:06:36 -0000
***************
*** 154,159 ****
--- 154,160 ----
       * Must always set snapshot for plannable queries.    Note we assume that
       * caller will take care of restoring ActiveSnapshot on exit/error.
       */
+     FreeSnapshot(ActiveSnapshot);
      ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());

      /*
***************
*** 603,608 ****
--- 604,611 ----

          /* Restore global vars and propagate error */
          ActivePortal = saveActivePortal;
+         if (portal->strategy == PORTAL_ONE_SELECT)
+             FreeSnapshot(ActiveSnapshot);
          ActiveSnapshot = saveActiveSnapshot;
          CurrentResourceOwner = saveResourceOwner;
          PortalContext = savePortalContext;
***************
*** 614,619 ****
--- 617,624 ----
      MemoryContextSwitchTo(oldContext);

      ActivePortal = saveActivePortal;
+     if (portal->strategy == PORTAL_ONE_SELECT)
+         FreeSnapshot(ActiveSnapshot);
      ActiveSnapshot = saveActiveSnapshot;
      CurrentResourceOwner = saveResourceOwner;
      PortalContext = savePortalContext;
***************
*** 1163,1169 ****
--- 1168,1177 ----
            IsA(utilityStmt, NotifyStmt) ||
            IsA(utilityStmt, UnlistenStmt) ||
            IsA(utilityStmt, CheckPointStmt)))
+     {
+         FreeSnapshot(ActiveSnapshot);
          ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
+     }
      else
          ActiveSnapshot = NULL;

***************
*** 1177,1184 ****
      /* Some utility statements may change context on us */
      MemoryContextSwitchTo(PortalGetHeapMemory(portal));

!     if (ActiveSnapshot)
!         FreeSnapshot(ActiveSnapshot);
      ActiveSnapshot = NULL;
  }

--- 1185,1191 ----
      /* Some utility statements may change context on us */
      MemoryContextSwitchTo(PortalGetHeapMemory(portal));

!     FreeSnapshot(ActiveSnapshot);
      ActiveSnapshot = NULL;
  }

Index: src/backend/utils/adt/ri_triggers.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/ri_triggers.c,v
retrieving revision 1.93
diff -c -c -r1.93 ri_triggers.c
*** src/backend/utils/adt/ri_triggers.c    25 Mar 2007 19:45:14 -0000    1.93
--- src/backend/utils/adt/ri_triggers.c    26 Mar 2007 21:06:38 -0000
***************
*** 2636,2642 ****
      char        workmembuf[32];
      int            spi_result;
      SPIPlanPtr    qplan;
!
      /*
       * Check to make sure current user has enough permissions to do the test
       * query.  (If not, caller can fall back to the trigger method, which
--- 2636,2643 ----
      char        workmembuf[32];
      int            spi_result;
      SPIPlanPtr    qplan;
!     Snapshot    snapshot;
!
      /*
       * Check to make sure current user has enough permissions to do the test
       * query.  (If not, caller can fall back to the trigger method, which
***************
*** 2770,2780 ****
       * really haven't got much choice.)  We need at most one tuple returned,
       * so pass limit = 1.
       */
      spi_result = SPI_execute_snapshot(qplan,
                                        NULL, NULL,
!                                       CopySnapshot(GetLatestSnapshot()),
                                        InvalidSnapshot,
                                        true, 1);

      /* Check result */
      if (spi_result != SPI_OK_SELECT)
--- 2771,2783 ----
       * really haven't got much choice.)  We need at most one tuple returned,
       * so pass limit = 1.
       */
+     snapshot = CopySnapshot(GetLatestSnapshot());
      spi_result = SPI_execute_snapshot(qplan,
                                        NULL, NULL,
!                                       snapshot,
                                        InvalidSnapshot,
                                        true, 1);
+     FreeSnapshot(snapshot);

      /* Check result */
      if (spi_result != SPI_OK_SELECT)
***************
*** 3310,3315 ****
--- 3313,3322 ----
                                        test_snapshot, crosscheck_snapshot,
                                        false, limit);

+     if (test_snapshot != InvalidSnapshot)
+         FreeSnapshot(test_snapshot);
+     if (crosscheck_snapshot != InvalidSnapshot)
+         FreeSnapshot(crosscheck_snapshot);
      /* Restore UID */
      SetUserId(save_uid);

Index: src/backend/utils/time/tqual.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/time/tqual.c,v
retrieving revision 1.102
diff -c -c -r1.102 tqual.c
*** src/backend/utils/time/tqual.c    25 Mar 2007 19:45:14 -0000    1.102
--- src/backend/utils/time/tqual.c    26 Mar 2007 21:06:38 -0000
***************
*** 65,70 ****
--- 65,79 ----
  Snapshot    LatestSnapshot = NULL;

  /*
+  * We use OpenSnapshotCount to record if there are any open snapshots.  If
+  * there are not, we can update MyProc->xmin. A more sophisticated solution
+  * would be to keep track of the oldest open snapshot and prevent
+  * MyProc->xmin from going below that value, but the complexity doesn't
+  * seem worth it.
+  */
+ static int OpenSnapshotCount = 0;
+
+ /*
   * This pointer is not maintained by this module, but it's convenient
   * to declare it here anyway.  Callers typically assign a copy of
   * GetTransactionSnapshot's result to ActiveSnapshot.
***************
*** 1224,1237 ****
      /* First call in transaction? */
      if (SerializableSnapshot == NULL)
      {
!         SerializableSnapshot = GetSnapshotData(&SerializableSnapshotData, true);
          return SerializableSnapshot;
      }

      if (IsXactIsoLevelSerializable)
          return SerializableSnapshot;

!     LatestSnapshot = GetSnapshotData(&LatestSnapshotData, false);

      return LatestSnapshot;
  }
--- 1233,1246 ----
      /* First call in transaction? */
      if (SerializableSnapshot == NULL)
      {
!         SerializableSnapshot = GetSnapshotData(&SerializableSnapshotData);
          return SerializableSnapshot;
      }

      if (IsXactIsoLevelSerializable)
          return SerializableSnapshot;

!     LatestSnapshot = GetSnapshotData(&LatestSnapshotData);

      return LatestSnapshot;
  }
***************
*** 1248,1254 ****
      if (SerializableSnapshot == NULL)
          elog(ERROR, "no snapshot has been set");

!     LatestSnapshot = GetSnapshotData(&LatestSnapshotData, false);

      return LatestSnapshot;
  }
--- 1257,1263 ----
      if (SerializableSnapshot == NULL)
          elog(ERROR, "no snapshot has been set");

!     LatestSnapshot = GetSnapshotData(&LatestSnapshotData);

      return LatestSnapshot;
  }
***************
*** 1266,1271 ****
--- 1275,1282 ----
      Size        subxipoff;
      Size        size;

+     OpenSnapshotCount++;
+
      /* We allocate any XID arrays needed in the same palloc block. */
      size = subxipoff = sizeof(SnapshotData) +
          snapshot->xcnt * sizeof(TransactionId);
***************
*** 1310,1316 ****
  void
  FreeSnapshot(Snapshot snapshot)
  {
!     pfree(snapshot);
  }

  /*
--- 1321,1337 ----
  void
  FreeSnapshot(Snapshot snapshot)
  {
!     if (snapshot != NULL)
!     {
!         OpenSnapshotCount--;
!         Assert(OpenSnapshotCount >= 0);
!         pfree(snapshot);
!     }
! }
!
! int GetOpenSnapshotCount(void)
! {
!     return OpenSnapshotCount;
  }

  /*
***************
*** 1328,1333 ****
--- 1349,1355 ----
      SerializableSnapshot = NULL;
      LatestSnapshot = NULL;
      ActiveSnapshot = NULL;        /* just for cleanliness */
+     OpenSnapshotCount = 0;
  }

  /*
Index: src/include/utils/tqual.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/tqual.h,v
retrieving revision 1.66
diff -c -c -r1.66 tqual.h
*** src/include/utils/tqual.h    25 Mar 2007 19:45:14 -0000    1.66
--- src/include/utils/tqual.h    26 Mar 2007 21:06:39 -0000
***************
*** 150,157 ****
  extern Snapshot CopySnapshot(Snapshot snapshot);
  extern void FreeSnapshot(Snapshot snapshot);
  extern void FreeXactSnapshot(void);

  /* in procarray.c; declared here to avoid including tqual.h in procarray.h: */
! extern Snapshot GetSnapshotData(Snapshot snapshot, bool serializable);

  #endif   /* TQUAL_H */
--- 150,158 ----
  extern Snapshot CopySnapshot(Snapshot snapshot);
  extern void FreeSnapshot(Snapshot snapshot);
  extern void FreeXactSnapshot(void);
+ extern int GetOpenSnapshotCount(void);

  /* in procarray.c; declared here to avoid including tqual.h in procarray.h: */
! extern Snapshot GetSnapshotData(Snapshot snapshot);

  #endif   /* TQUAL_H */
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.191
diff -c -c -r1.191 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c    25 Mar 2007 23:27:59 -0000    1.191
--- src/pl/plpgsql/src/pl_exec.c    26 Mar 2007 21:06:41 -0000
***************
*** 4129,4139 ****
--- 4129,4143 ----
      PG_CATCH();
      {
          /* Restore global vars and propagate error */
+         if (!estate->readonly_func)
+             FreeSnapshot(ActiveSnapshot);
          ActiveSnapshot = saveActiveSnapshot;
          PG_RE_THROW();
      }
      PG_END_TRY();

+     if (!estate->readonly_func)
+         FreeSnapshot(ActiveSnapshot);
      ActiveSnapshot = saveActiveSnapshot;
      SPI_pop();