Tom Lane wrote: > Heikki Linnakangas <[EMAIL PROTECTED]> writes: > > It seems to me a lot cleaner to do the reference counting like Tom > > suggested. Increase the refcount on CopySnapshot, and decrease it on > > FreeSnapshot. Assuming that all callers of CopySnapshot free the > > snapshot with FreeSnapshot when they're done with it. > > I don't believe we bother at the moment; which is one of the reasons > it'd be a nontrivial patch. I do think it might be worth doing though. > In the simple case where you're just issuing successive non-cursor > commands within a READ COMMITTED transaction, a refcounted > implementation would be able to recognize that there are *no* live > snapshots between commands and therefore reset MyProc->xmin to 0 > whenever the backend is idle.
Attached is my current version of the patch. It doesn't work now that I tried to do reference count for Snapshots, but will stop now that Tom is considering redesigning the snapshot mechanism. > OTOH, do we have any evidence that this is worth bothering with at all? > I fear that the cases of long-running transactions that are problems > in the real world wouldn't be helped much --- for instance, pg_dump > wouldn't change behavior because it uses a serializable transaction. > Also, at some point a long-running transaction becomes a bottleneck > simply because its XID is itself the oldest thing visible in the > ProcArray and is determining everyone's xmin. How much daylight is > there really between "your xmin is old" and "your xid is old"? Well, interesting you mention that, because I have a second idea on how to improve things. We start with MyProc->xmin equal to our own xid, and then look for earlier transactions. It should be possible to skip considering our own xid for MyProc->xmin. This would obviously help VACUUM during long-running transactions. While our transaction is running, our xid isn't committed, so VACUUM isn't going to touch any of our rows, and if other transactions complete before our multi-transaction _statement_ starts, we can't see deleted rows from them transaction, so why keep the deleted rows around? Consider this case: Session #: 1 2 3 BEGIN; SELECT 1; CREATE TABLE test(x int); INSERT INTO test VALUES (1); DELETE FROM test; SELECT 1; VACUUM VERBOSE test; (row can be reused) COMMIT; VACUUM VERBOSE test; (normal row reuse) As I understand it, in READ COMMITTED mode, we have to skip transactions in progress when our _statement_ starts, but anything committed before that we see and we don't see dead rows created by them. -- 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.280 diff -c -c -r1.280 index.c *** src/backend/catalog/index.c 3 Mar 2007 20:08:41 -0000 1.280 --- src/backend/catalog/index.c 24 Mar 2007 19:17:56 -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 24 Mar 2007 19:17:56 -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 24 Mar 2007 19:17:56 -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 24 Mar 2007 19:17:57 -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 24 Mar 2007 19:17:59 -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 24 Mar 2007 19:17:59 -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 24 Mar 2007 19:18:00 -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.22 diff -c -c -r1.22 procarray.c *** src/backend/storage/ipc/procarray.c 23 Mar 2007 03:16:39 -0000 1.22 --- src/backend/storage/ipc/procarray.c 24 Mar 2007 19:18:01 -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 24 Mar 2007 19:18:01 -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 24 Mar 2007 19:18:02 -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 24 Mar 2007 19:18:02 -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()); /* *************** *** 486,491 **** --- 487,493 ---- * Must set snapshot before starting executor. Be sure to * copy it into the portal's context. */ + FreeSnapshot(ActiveSnapshot); if (snapshot) ActiveSnapshot = CopySnapshot(snapshot); else *************** *** 1163,1169 **** --- 1165,1174 ---- 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; } --- 1182,1188 ---- /* 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.92 diff -c -c -r1.92 ri_triggers.c *** src/backend/utils/adt/ri_triggers.c 15 Mar 2007 23:12:06 -0000 1.92 --- src/backend/utils/adt/ri_triggers.c 24 Mar 2007 19:18:04 -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.101 diff -c -c -r1.101 tqual.c *** src/backend/utils/time/tqual.c 5 Jan 2007 22:19:47 -0000 1.101 --- src/backend/utils/time/tqual.c 24 Mar 2007 19:18:04 -0000 *************** *** 54,59 **** --- 54,67 ---- static SnapshotData SnapshotDirtyData; static SnapshotData SerializableSnapshotData; static SnapshotData LatestSnapshotData; + /* + * 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; /* Externally visible pointers to valid snapshots: */ Snapshot SnapshotDirty = &SnapshotDirtyData; *************** *** 1207,1220 **** /* First call in transaction? */ if (SerializableSnapshot == NULL) { ! SerializableSnapshot = GetSnapshotData(&SerializableSnapshotData, true); return SerializableSnapshot; } if (IsXactIsoLevelSerializable) return SerializableSnapshot; ! LatestSnapshot = GetSnapshotData(&LatestSnapshotData, false); return LatestSnapshot; } --- 1215,1228 ---- /* First call in transaction? */ if (SerializableSnapshot == NULL) { ! SerializableSnapshot = GetSnapshotData(&SerializableSnapshotData); return SerializableSnapshot; } if (IsXactIsoLevelSerializable) return SerializableSnapshot; ! LatestSnapshot = GetSnapshotData(&LatestSnapshotData); return LatestSnapshot; } *************** *** 1231,1237 **** if (SerializableSnapshot == NULL) elog(ERROR, "no snapshot has been set"); ! LatestSnapshot = GetSnapshotData(&LatestSnapshotData, false); return LatestSnapshot; } --- 1239,1245 ---- if (SerializableSnapshot == NULL) elog(ERROR, "no snapshot has been set"); ! LatestSnapshot = GetSnapshotData(&LatestSnapshotData); return LatestSnapshot; } *************** *** 1251,1256 **** --- 1259,1266 ---- Size subxipoff; Size size; + OpenSnapshotCount++; + /* We allocate any XID arrays needed in the same palloc block. */ size = subxipoff = sizeof(SnapshotData) + snapshot->xcnt * sizeof(TransactionId); *************** *** 1295,1301 **** void FreeSnapshot(Snapshot snapshot) { ! pfree(snapshot); } /* --- 1305,1320 ---- void FreeSnapshot(Snapshot snapshot) { ! if (snapshot != NULL) ! { ! OpenSnapshotCount--; ! pfree(snapshot); ! } ! } ! ! int GetOpenSnapshotCount(void) ! { ! return OpenSnapshotCount; } /* *************** *** 1312,1318 **** --- 1331,1339 ---- */ SerializableSnapshot = NULL; LatestSnapshot = NULL; + FreeSnapshot(ActiveSnapshot); ActiveSnapshot = NULL; /* just for cleanliness */ + Assert(GetOpenSnapshotCount() == 0); } /* Index: src/include/utils/tqual.h =================================================================== RCS file: /cvsroot/pgsql/src/include/utils/tqual.h,v retrieving revision 1.65 diff -c -c -r1.65 tqual.h *** src/include/utils/tqual.h 5 Jan 2007 22:19:59 -0000 1.65 --- src/include/utils/tqual.h 24 Mar 2007 19:18:05 -0000 *************** *** 147,154 **** 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 */ --- 147,155 ---- 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.190 diff -c -c -r1.190 pl_exec.c *** src/pl/plpgsql/src/pl_exec.c 15 Mar 2007 23:12:07 -0000 1.190 --- src/pl/plpgsql/src/pl_exec.c 24 Mar 2007 19:18:06 -0000 *************** *** 4139,4149 **** --- 4139,4153 ---- 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 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq