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
