diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index de3f965b37a4be93913907b1683c07ea24b4e633..3dab45c2da60a1010d566fccf658a4a55ee3ece3 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1906,9 +1906,6 @@ CommitTransaction(void) /* Clean up the relation cache */ AtEOXact_RelationCache(true); - /* Clean up the snapshot manager */ - AtEarlyCommit_Snapshot(); - /* * Make catalog changes visible to all backends. This has to happen after * relcache references are dropped (see comments for @@ -2158,9 +2155,6 @@ PrepareTransaction(void) /* Clean up the relation cache */ AtEOXact_RelationCache(true); - /* Clean up the snapshot manager */ - AtEarlyCommit_Snapshot(); - /* notify doesn't need a postprepare call */ PostPrepare_PgStat(); @@ -2339,7 +2333,6 @@ AbortTransaction(void) AtEOXact_ComboCid(); AtEOXact_HashTables(false); AtEOXact_PgStat(false); - AtEOXact_Snapshot(false); pgstat_report_xact_timestamp(0); } @@ -2368,6 +2361,7 @@ CleanupTransaction(void) * do abort cleanup processing */ AtCleanup_Portals(); /* now safe to release portal memory */ + AtEOXact_Snapshot(false); /* and release the transaction's snapshots */ CurrentResourceOwner = NULL; /* and resource owner */ if (TopTransactionResourceOwner) diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c index 1754180453daaa54e732bcd82cae5f3b70631524..d39f8975f8816d9bba02c0c398323ca470f1340b 100644 --- a/src/backend/storage/lmgr/predicate.c +++ b/src/backend/storage/lmgr/predicate.c @@ -146,7 +146,7 @@ * PageIsPredicateLocked(Relation relation, BlockNumber blkno) * * predicate lock maintenance - * RegisterSerializableTransaction(Snapshot snapshot) + * GetSerializableTransactionSnapshot(Snapshot snapshot) * RegisterPredicateLockingXid(void) * PredicateLockRelation(Relation relation, Snapshot snapshot) * PredicateLockPage(Relation relation, BlockNumber blkno, @@ -417,7 +417,7 @@ static void OldSerXidSetActiveSerXmin(TransactionId xid); static uint32 predicatelock_hash(const void *key, Size keysize); static void SummarizeOldestCommittedSxact(void); static Snapshot GetSafeSnapshot(Snapshot snapshot); -static Snapshot RegisterSerializableTransactionInt(Snapshot snapshot); +static Snapshot GetSerializableTransactionSnapshotInt(Snapshot snapshot); static bool PredicateLockExists(const PREDICATELOCKTARGETTAG *targettag); static bool GetParentPredicateLockTag(const PREDICATELOCKTARGETTAG *tag, PREDICATELOCKTARGETTAG *parent); @@ -1485,6 +1485,10 @@ SummarizeOldestCommittedSxact(void) * without further checks. This requires waiting for concurrent * transactions to complete, and retrying with a new snapshot if * one of them could possibly create a conflict. + * + * As with GetSerializableTransactionSnapshot (which this is a subroutine + * for), the passed-in Snapshot pointer should reference a static data + * area that can safely be passed to GetSnapshotData. */ static Snapshot GetSafeSnapshot(Snapshot origSnapshot) @@ -1496,12 +1500,12 @@ GetSafeSnapshot(Snapshot origSnapshot) while (true) { /* - * RegisterSerializableTransactionInt is going to call - * GetSnapshotData, so we need to provide it the static snapshot our - * caller passed to us. It returns a copy of that snapshot and - * registers it on TopTransactionResourceOwner. + * GetSerializableTransactionSnapshotInt is going to call + * GetSnapshotData, so we need to provide it the static snapshot area + * our caller passed to us. The pointer returned is actually the same + * one passed to it, but we avoid assuming that here. */ - snapshot = RegisterSerializableTransactionInt(origSnapshot); + snapshot = GetSerializableTransactionSnapshotInt(origSnapshot); if (MySerializableXact == InvalidSerializableXact) return snapshot; /* no concurrent r/w xacts; it's safe */ @@ -1535,8 +1539,6 @@ GetSafeSnapshot(Snapshot origSnapshot) (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), errmsg("deferrable snapshot was unsafe; trying a new one"))); ReleasePredicateLocks(false); - UnregisterSnapshotFromOwner(snapshot, - TopTransactionResourceOwner); } /* @@ -1549,28 +1551,34 @@ GetSafeSnapshot(Snapshot origSnapshot) } /* - * Acquire and register a snapshot which can be used for this transaction.. + * Acquire a snapshot that can be used for the current transaction. + * * Make sure we have a SERIALIZABLEXACT reference in MySerializableXact. * It should be current for this process and be contained in PredXact. + * + * The passed-in Snapshot pointer should reference a static data area that + * can safely be passed to GetSnapshotData. The return value is actually + * always this same pointer; no new snapshot data structure is allocated + * within this function. */ Snapshot -RegisterSerializableTransaction(Snapshot snapshot) +GetSerializableTransactionSnapshot(Snapshot snapshot) { Assert(IsolationIsSerializable()); /* * A special optimization is available for SERIALIZABLE READ ONLY * DEFERRABLE transactions -- we can wait for a suitable snapshot and - * thereby avoid all SSI overhead once it's running.. + * thereby avoid all SSI overhead once it's running. */ if (XactReadOnly && XactDeferrable) return GetSafeSnapshot(snapshot); - return RegisterSerializableTransactionInt(snapshot); + return GetSerializableTransactionSnapshotInt(snapshot); } static Snapshot -RegisterSerializableTransactionInt(Snapshot snapshot) +GetSerializableTransactionSnapshotInt(Snapshot snapshot) { PGPROC *proc; VirtualTransactionId vxid; @@ -1607,9 +1615,8 @@ RegisterSerializableTransactionInt(Snapshot snapshot) } } while (!sxact); - /* Get and register a snapshot */ + /* Get the snapshot */ snapshot = GetSnapshotData(snapshot); - snapshot = RegisterSnapshotOnOwner(snapshot, TopTransactionResourceOwner); /* * If there are no serializable transactions which are not read-only, we diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index bb25ac6ab2c464480258335297ff4500c049bda6..518aaf1af0cfa66f88d3b19272d3118f172c15c0 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -7,6 +7,14 @@ * persistent memory. When a snapshot is no longer in any of these lists * (tracked by separate refcounts on each snapshot), its memory can be freed. * + * The FirstXactSnapshot, if any, is treated a bit specially: we increment its + * regd_count and count it in RegisteredSnapshots, but this reference is not + * tracked by a resource owner. We used to use the TopTransactionResourceOwner + * to track this snapshot reference, but that introduces logical circularity + * and thus makes it impossible to clean up in a sane fashion. It's better to + * handle this reference as an internally-tracked registration, so that this + * module is entirely lower-level than ResourceOwners. + * * These arrangements let us reset MyProc->xmin when there are no snapshots * referenced by this transaction. (One possible improvement would be to be * able to advance Xmin when the snapshot with the earliest Xmin is no longer @@ -97,11 +105,11 @@ static int RegisteredSnapshots = 0; bool FirstSnapshotSet = false; /* - * Remembers whether this transaction registered a transaction snapshot at - * start. We cannot trust FirstSnapshotSet in combination with - * IsolationUsesXactSnapshot(), because GUC may be reset before us. + * Remember the serializable transaction snapshot, if any. We cannot trust + * FirstSnapshotSet in combination with IsolationUsesXactSnapshot(), because + * GUC may be reset before us, changing the value of IsolationUsesXactSnapshot. */ -static bool registered_xact_snapshot = false; +static Snapshot FirstXactSnapshot = NULL; static Snapshot CopySnapshot(Snapshot snapshot); @@ -125,23 +133,27 @@ GetTransactionSnapshot(void) if (!FirstSnapshotSet) { Assert(RegisteredSnapshots == 0); + Assert(FirstXactSnapshot == NULL); /* * In transaction-snapshot mode, the first snapshot must live until * end of xact regardless of what the caller does with it, so we must - * register it internally here and unregister it at end of xact. + * make a copy of it rather than returning CurrentSnapshotData + * directly. */ if (IsolationUsesXactSnapshot()) { + /* First, create the snapshot in CurrentSnapshotData */ if (IsolationIsSerializable()) - CurrentSnapshot = RegisterSerializableTransaction(&CurrentSnapshotData); + CurrentSnapshot = GetSerializableTransactionSnapshot(&CurrentSnapshotData); else - { CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData); - CurrentSnapshot = RegisterSnapshotOnOwner(CurrentSnapshot, - TopTransactionResourceOwner); - } - registered_xact_snapshot = true; + /* Make a saved copy */ + CurrentSnapshot = CopySnapshot(CurrentSnapshot); + FirstXactSnapshot = CurrentSnapshot; + /* Mark it as "registered" in FirstXactSnapshot */ + FirstXactSnapshot->regd_count++; + RegisteredSnapshots++; } else CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData); @@ -522,26 +534,6 @@ AtSubAbort_Snapshot(int level) SnapshotResetXmin(); } -/* - * AtEarlyCommit_Snapshot - * - * Snapshot manager's cleanup function, to be called on commit, before - * doing resowner.c resource release. - */ -void -AtEarlyCommit_Snapshot(void) -{ - /* - * In transaction-snapshot mode we must unregister our private refcount to - * the transaction-snapshot. - */ - if (registered_xact_snapshot) - UnregisterSnapshotFromOwner(CurrentSnapshot, - TopTransactionResourceOwner); - registered_xact_snapshot = false; - -} - /* * AtEOXact_Snapshot * Snapshot manager's cleanup function for end of transaction @@ -549,6 +541,23 @@ AtEarlyCommit_Snapshot(void) void AtEOXact_Snapshot(bool isCommit) { + /* + * In transaction-snapshot mode we must release our privately-managed + * reference to the transaction snapshot. We must decrement + * RegisteredSnapshots to keep the check below happy. But we don't bother + * to do FreeSnapshot, for two reasons: the memory will go away with + * TopTransactionContext anyway, and if someone has left the snapshot + * stacked as active, we don't want the code below to be chasing through + * a dangling pointer. + */ + if (FirstXactSnapshot != NULL) + { + Assert(FirstXactSnapshot->regd_count > 0); + Assert(RegisteredSnapshots > 0); + RegisteredSnapshots--; + } + FirstXactSnapshot = NULL; + /* On commit, complain about leftover snapshots */ if (isCommit) { @@ -574,5 +583,6 @@ AtEOXact_Snapshot(bool isCommit) SecondarySnapshot = NULL; FirstSnapshotSet = false; - registered_xact_snapshot = false; + + SnapshotResetXmin(); } diff --git a/src/include/storage/predicate.h b/src/include/storage/predicate.h index 5ddbc1d3f413506fcd4dff851a5960c112f227ea..9603b10ad4052e55109b976d521d6054b656dc8a 100644 --- a/src/include/storage/predicate.h +++ b/src/include/storage/predicate.h @@ -42,7 +42,7 @@ extern void CheckPointPredicate(void); extern bool PageIsPredicateLocked(Relation relation, BlockNumber blkno); /* predicate lock maintenance */ -extern Snapshot RegisterSerializableTransaction(Snapshot snapshot); +extern Snapshot GetSerializableTransactionSnapshot(Snapshot snapshot); extern void RegisterPredicateLockingXid(TransactionId xid); extern void PredicateLockRelation(Relation relation, Snapshot snapshot); extern void PredicateLockPage(Relation relation, BlockNumber blkno, Snapshot snapshot); diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h index c969a37796173bdab05925d3a30364b3fe1bde7e..e665a28aff8571e50ce318d8a0ca4710ec503b3c 100644 --- a/src/include/utils/snapmgr.h +++ b/src/include/utils/snapmgr.h @@ -40,7 +40,6 @@ extern void UnregisterSnapshotFromOwner(Snapshot snapshot, ResourceOwner owner); extern void AtSubCommit_Snapshot(int level); extern void AtSubAbort_Snapshot(int level); -extern void AtEarlyCommit_Snapshot(void); extern void AtEOXact_Snapshot(bool isCommit); #endif /* SNAPMGR_H */