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 <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] so that your > message can get through to the mailing list cleanly -- Bruce Momjian <[EMAIL PROTECTED]> 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();
---------------------------(end of broadcast)--------------------------- TIP 5: don't forget to increase your free space map settings