While working on the CSN snapshot patch, I got sidetracked looking
closer into the snapshot tracking in snapmgr.c. Attached are a few
patches to clarify some things.
# Patch 1: Remove unnecessary GetTransactionSnapshot() calls and FIXME
comments
In commit dc7420c2c927, Andres added FIXME comments like these in a few
places:
autovacuum.c, get_database_list(void):
/*
* Start a transaction so we can access pg_database, and get a snapshot.
* We don't have a use for the snapshot itself, but we're interested in
* the secondary effect that it sets RecentGlobalXmin. (This is
critical
* for anything that reads heap pages, because HOT may decide to prune
* them even if the process doesn't attempt to modify any tuples.)
*
* FIXME: This comment is inaccurate / the code buggy. A snapshot that
is
* not pushed/active does not reliably prevent HOT pruning (->xmin could
* e.g. be cleared when cache invalidations are processed).
*/
StartTransactionCommand();
(void) GetTransactionSnapshot();
Those GetTransactionSnapshot() calls are unnecessary, because we hold
onto registered copy of CatalogSnapshot throughout the catalog scans.
This patch removes those unnecessary calls, and the FIXMEs.
# Patch 2: Assert that a snapshot is active or registered before it's used
GetTransactionSnapshot() comment said:
* Note that the return value may point at static storage that will be modified
* by future calls and by CommandCounterIncrement(). Callers should call
* RegisterSnapshot or PushActiveSnapshot on the returned snap if it is to be
* used very long.
That's pretty vague. Firstly, it says the returned value _may_ point to
static storage, but ISTM it _always_ does, if you interpret "static
storage" liberally. Some callers actually rely on the fact that you can
call GetTransactionSnapshot() and throw away the result without having a
leak. So I propose rewording that to "return value points at static
storage", rather than just "may point".
In REPEATABLE READ mode, the returned CurrentSnapshot is palloc'd, not a
pointer directly to a static variable, but all calls within the same
transaction return the same palloc'd Snapshot pointer, and will be
modified by CommandCounterIncrement(). From the caller's point of view,
it's like a static.
Secondly, what exactly is "used very long"? It means until the next call
of any of the Get*Snapshot() functions, CommandCounterIncrement(), or
anything that might call SnapshotResetXmin() like PopActiveSnapshot().
Given how complicated that gets, I feel it's dangerous to do pretty much
anything else than immediately call PushActiveSnapshot() or
RegisterSnapshot() with it. To try to enforce that, this patch adds an
assertion in HeapTupleSatisfiesMVCC() that the snapshot must be
registered or pushed active. That's not a very accurate check of that
stricter rule: some callers were violating the new assertion and had
comments to explain why it was safe, and OTOH it won't catch calls to
those invalidating functions that don't involve visibility checks.
We were violating that assertion in a few places, which were not wrong
and had explaining comments, but this patch changes them to just
register the snapshot instead of explaining why it's safe to skip it.
# Patch 3: Add comment with more details on active snapshots
Now that I have this swapped in my head, I wrote a few paragraphs on how
the active snapshot stack works at high level.
# Patch 4: Add checks that no snapshots are "leaked"
This patch is not to be committed right now, just for discussion.
I'm not very happy with how GetTransactionSnapshot() and friends return
a statically allocated snapshot. The whole "return value should not be
used very long" thing is just so vague. If we changed it to return a
palloc'd snapshot, would we introduce leaks? This patch adds assertions
that every call to GetTransactionSnapshot() is paired with a
PushActiveSnapshot() or RegsiterSnapshot() call, and changes a few
places that were violating that stricter rule. Some of those changes
seem nice anyway, like registering the snapshot in verify_heapam(), even
though they're not strictly necessary today.
A perhaps better way to enforce that would be to replace
GetTransactionSnapshot() with functions that also push or register the
snapshot:
RegisterSnapshot(GetTransactionSnapshot()) -> RegisterTransactionSnapshot()
PushActiveSnapshot(GetTransactionSnapshot()) -> PushTransactionSnapshot()
That function signature would eliminate the concept of a returned
statically-allocated snapshot, and the whole question of what does "used
very long" mean in GetTransactionSnapshot(). Thoughts on that?
--
Heikki Linnakangas
Neon (https://neon.tech)
From 7a32da753d05819c991d93cce3e3174f5a142238 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 4 Dec 2024 18:10:40 +0200
Subject: [PATCH 1/5] Remove unnecessary GetTransactionSnapshot() calls and
FIXME comments
In get_database_list() and get_subscription_list(), the
GetTransactionSnapshot() call is not required because the catalog
table scans use the catalog snapshot, which is held until the end of
the scan. See table_beginscan_catalog(), which calls
RegisterSnapshot(GetCatalogSnapshot(relid)).
In InitPostgres, it's a little less obvious that it's not required,
but still true I believe. All the catalog lookups in InitPostgres()
also use the catalog snapshot, and looked up values are copied.
Furthermore, as the removed comments said, calling
GetTransactionSnapshot() didn't really prevent MyProc->xmin from being
reset anyway.
---
src/backend/postmaster/autovacuum.c | 11 +----------
src/backend/replication/logical/launcher.c | 11 +----------
src/backend/utils/init/postinit.c | 13 +------------
3 files changed, 3 insertions(+), 32 deletions(-)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index dc3cf87abab..8078eeef62e 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1799,18 +1799,9 @@ get_database_list(void)
resultcxt = CurrentMemoryContext;
/*
- * Start a transaction so we can access pg_database, and get a snapshot.
- * We don't have a use for the snapshot itself, but we're interested in
- * the secondary effect that it sets RecentGlobalXmin. (This is critical
- * for anything that reads heap pages, because HOT may decide to prune
- * them even if the process doesn't attempt to modify any tuples.)
- *
- * FIXME: This comment is inaccurate / the code buggy. A snapshot that is
- * not pushed/active does not reliably prevent HOT pruning (->xmin could
- * e.g. be cleared when cache invalidations are processed).
+ * Start a transaction so we can access pg_database.
*/
StartTransactionCommand();
- (void) GetTransactionSnapshot();
rel = table_open(DatabaseRelationId, AccessShareLock);
scan = table_beginscan_catalog(rel, 0, NULL);
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index e5fdca8bbf6..8b196420445 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -121,18 +121,9 @@ get_subscription_list(void)
resultcxt = CurrentMemoryContext;
/*
- * Start a transaction so we can access pg_database, and get a snapshot.
- * We don't have a use for the snapshot itself, but we're interested in
- * the secondary effect that it sets RecentGlobalXmin. (This is critical
- * for anything that reads heap pages, because HOT may decide to prune
- * them even if the process doesn't attempt to modify any tuples.)
- *
- * FIXME: This comment is inaccurate / the code buggy. A snapshot that is
- * not pushed/active does not reliably prevent HOT pruning (->xmin could
- * e.g. be cleared when cache invalidations are processed).
+ * Start a transaction so we can access pg_subscription.
*/
StartTransactionCommand();
- (void) GetTransactionSnapshot();
rel = table_open(SubscriptionRelationId, AccessShareLock);
scan = table_beginscan_catalog(rel, 0, NULL);
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 5b657a3f135..770ab6906e7 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -813,16 +813,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
}
/*
- * Start a new transaction here before first access to db, and get a
- * snapshot. We don't have a use for the snapshot itself, but we're
- * interested in the secondary effect that it sets RecentGlobalXmin. (This
- * is critical for anything that reads heap pages, because HOT may decide
- * to prune them even if the process doesn't attempt to modify any
- * tuples.)
- *
- * FIXME: This comment is inaccurate / the code buggy. A snapshot that is
- * not pushed/active does not reliably prevent HOT pruning (->xmin could
- * e.g. be cleared when cache invalidations are processed).
+ * Start a new transaction here before first access to db.
*/
if (!bootstrap)
{
@@ -837,8 +828,6 @@ InitPostgres(const char *in_dbname, Oid dboid,
* Fortunately, "read committed" is plenty good enough.
*/
XactIsoLevel = XACT_READ_COMMITTED;
-
- (void) GetTransactionSnapshot();
}
/*
--
2.39.5
From f6d9c033a1e686b1600a435c857e728f98901997 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 13 Dec 2024 14:26:07 +0200
Subject: [PATCH 2/5] Assert that a snapshot is active or registered before
it's used
This is to catch potential bugs where a snapshot might get invalidated
while it's still in use. No such bugs were found by this, but the
advice in GetTransactionSnapshot() that you "should call
RegisterSnapshot or PushActiveSnapshot on the returned snap if it is
to be used very long" felt too unclear to me.
Fix a few cases that were playing fast and loose with that and just
assumed that the snapshot cannot be invalidated during a scan. Those
assumptions were not wrong, but they're not performance critical, so
let's drop the excuses and just register the snapshot. This allows us
to have an assertion in HeapTupleSatisfiesMVCC that the snapshot is
appropriately registered.
Adjust the comment in GetTransactionSnapshot() a little, because in a
few places we rely on the fact that GetTransactionSnapshot() returns a
statically allocated Snapshot; we'd have leaks otherwise.
---
src/backend/access/heap/heapam_visibility.c | 9 +++++++++
src/backend/access/index/genam.c | 8 ++------
src/backend/commands/dbcommands.c | 3 ++-
src/backend/utils/cache/relcache.c | 15 +++++++++------
src/backend/utils/time/snapmgr.c | 8 ++++----
5 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 9243feed01f..6c0f872c730 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -962,6 +962,15 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
{
HeapTupleHeader tuple = htup->t_data;
+ /*
+ * Assert that the caller has registered the snapshot. This function
+ * doesn't care about the registration as such, but in general you
+ * shouldn't try to use a snapshot without registration because it might
+ * get invalidated while it's still in use, and this is a convenient place
+ * to check for that.
+ */
+ Assert(snapshot->regd_count > 0 || snapshot->active_count > 0);
+
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 4b4ebff6a17..b12bb6c8d70 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -576,17 +576,13 @@ systable_recheck_tuple(SysScanDesc sysscan, HeapTuple tup)
Assert(tup == ExecFetchSlotHeapTuple(sysscan->slot, false, NULL));
- /*
- * Trust that table_tuple_satisfies_snapshot() and its subsidiaries
- * (commonly LockBuffer() and HeapTupleSatisfiesMVCC()) do not themselves
- * acquire snapshots, so we need not register the snapshot. Those
- * facilities are too low-level to have any business scanning tables.
- */
freshsnap = GetCatalogSnapshot(RelationGetRelid(sysscan->heap_rel));
+ freshsnap = RegisterSnapshot(freshsnap);
result = table_tuple_satisfies_snapshot(sysscan->heap_rel,
sysscan->slot,
freshsnap);
+ UnregisterSnapshot(freshsnap);
/*
* Handle the concurrent abort while fetching the catalog tuple during
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index aa91a396967..034c5938c66 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -288,7 +288,7 @@ ScanSourceDatabasePgClass(Oid tbid, Oid dbid, char *srcpath)
* snapshot - or the active snapshot - might not be new enough for that,
* but the return value of GetLatestSnapshot() should work fine.
*/
- snapshot = GetLatestSnapshot();
+ snapshot = RegisterSnapshot(GetLatestSnapshot());
/* Process the relation block by block. */
for (blkno = 0; blkno < nblocks; blkno++)
@@ -313,6 +313,7 @@ ScanSourceDatabasePgClass(Oid tbid, Oid dbid, char *srcpath)
UnlockReleaseBuffer(buf);
}
+ UnregisterSnapshot(snapshot);
/* Release relation lock. */
UnlockRelationId(&relid, AccessShareLock);
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 422509f18d7..dc299ccb760 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -371,14 +371,13 @@ ScanPgRelation(Oid targetRelId, bool indexOK, bool force_non_historic)
pg_class_desc = table_open(RelationRelationId, AccessShareLock);
/*
- * The caller might need a tuple that's newer than the one the historic
- * snapshot; currently the only case requiring to do so is looking up the
- * relfilenumber of non mapped system relations during decoding. That
- * snapshot can't change in the midst of a relcache build, so there's no
- * need to register the snapshot.
+ * The caller might need a tuple that's newer than what's visible to the
+ * historic snapshot; currently the only case requiring to do so is
+ * looking up the relfilenumber of non mapped system relations during
+ * decoding.
*/
if (force_non_historic)
- snapshot = GetNonHistoricCatalogSnapshot(RelationRelationId);
+ snapshot = RegisterSnapshot(GetNonHistoricCatalogSnapshot(RelationRelationId));
pg_class_scan = systable_beginscan(pg_class_desc, ClassOidIndexId,
indexOK && criticalRelcachesBuilt,
@@ -395,6 +394,10 @@ ScanPgRelation(Oid targetRelId, bool indexOK, bool force_non_historic)
/* all done */
systable_endscan(pg_class_scan);
+
+ if (snapshot)
+ UnregisterSnapshot(snapshot);
+
table_close(pg_class_desc, AccessShareLock);
return pg_class_tuple;
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index a1a0c2adeb6..3c408762728 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -203,10 +203,10 @@ typedef struct SerializedSnapshotData
* GetTransactionSnapshot
* Get the appropriate snapshot for a new query in a transaction.
*
- * Note that the return value may point at static storage that will be modified
- * by future calls and by CommandCounterIncrement(). Callers should call
- * RegisterSnapshot or PushActiveSnapshot on the returned snap if it is to be
- * used very long.
+ * Note that the return value points at static storage that will be modified
+ * by future calls and by CommandCounterIncrement(). Callers must call
+ * RegisterSnapshot or PushActiveSnapshot on the returned snap before doing
+ * any other non-trivial work that could invalidate it.
*/
Snapshot
GetTransactionSnapshot(void)
--
2.39.5
From 0d56ca03b2f290ab8e38e775e8d0b19631233ea0 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 11 Dec 2024 14:22:54 +0200
Subject: [PATCH 3/5] Add comment with more details on active snapshots
---
src/backend/utils/time/snapmgr.c | 55 ++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 3c408762728..05f16666192 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -3,11 +3,66 @@
* snapmgr.c
* PostgreSQL snapshot manager
*
+ * The following functions return a snapshot that can be used in visibility
+ * checks:
+ *
+ * - GetTransactionSnapshot
+ * - GetLatestSnapshot
+ * - GetCatalogSnapshot
+ * - GetNonHistoricCatalogSnapshot
+ *
+ * All of these functions return a reference to a statically allocated
+ * snapshot, which must be copied and registered by calling
+ * PushActiveSnapshot() or RegisterSnapshot() before use.
+ *
+ * In addition to the above, there are some special snapshots, like
+ * SnapshotSelf, SnapshotAny, and "dirty" snapshots.
+ *
* We keep track of snapshots in two ways: those "registered" by resowner.c,
* and the "active snapshot" stack. All snapshots in either of them live in
* 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.
*
+ * ActiveSnapshot stack
+ * --------------------
+ *
+ * Most visibility checks use the current "active snapshot". When running
+ * normal queries, the active snapshot is set when query execution begins,
+ * depending on transaction isolation level.
+ *
+ * The active snapshot is tracked in a stack, so that the currently active one
+ * is at the top of the stack. It mirrors the process call stack: whenever we
+ * recurse or switch context to fetch rows from a different portal for
+ * example, the appropriate snapshot is pushed to become the active snapshot,
+ * and popped on return. Once upon a time, ActiveSnapshot was just a global
+ * variable that was saved and restored similar to CurrentMemoryContext, but
+ * nowadays it's managed as a separate data structure so that we can keep
+ * track of which snapshots are in use and reset MyProc->xmin when there is no
+ * active snapshot.
+ *
+ * However, there are a couple of exceptions where the active snapshot stack
+ * does not strictly mirror the call stack:
+ *
+ * - VACUUM and a few other utility commands manage their own transactions,
+ * which take their own snapshots. They are called with an active snapshot
+ * set, like most utility commands, but they pop the active snapshot that
+ * was pushed by the caller. PortalRunUtility knows about the possibility
+ * that the snapshot it pushed is no longer active on return.
+ *
+ * - When COMMIT or ROLLBACK is executed within a procedure or DO-block, the
+ * active snapshot stack is destroyed, and re-established later when
+ * subsequent statements in the procedure are executed. There are many
+ * limitations on when in-procedure COMMIT/ROLLBACK is allowed; one such
+ * limitation is that all the snapshots on the active snapshot stack are
+ * known to portals that are being executed, which makes it safe to reset
+ * the stack. See EnsurePortalSnapshotExists().
+ *
+ * Registered snapshots
+ * --------------------
+ *
+ * In addition to snapshots pushed to the active snapshot stack, a snapshot
+ * can be registered with a resource owner.
+ *
* The FirstXactSnapshot, if any, is treated a bit specially: we increment its
* regd_count and list it in RegisteredSnapshots, but this reference is not
* tracked by a resource owner. We used to use the TopTransactionResourceOwner
--
2.39.5
From 6b66d743b5113fe504260b6a230ae23d002c4d66 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 11 Dec 2024 23:59:49 +0200
Subject: [PATCH 4/5] WIP: Add checks that no snapshots are "leaked"
GetTransactionSnapshot() and friends currently return a pointer to a
statically allocated SnapshotData. That makes it OK to call
GetTransactionSnapshot() and throw away the result, without leaking
the snapshot. The comment on GetTransactionSnapshot() said that you
"the returned value *may* point to static storage", but we were
actually relying on that in a few places, to not leak.
This adds an assertion that every call to GetTransactionSnapshot() is
paired with a call to PushActiveSnapshot() or RegisterSnapshot(). With
this, GetTransactionSnapshot() coult return a dynamically allocated
Snapshot without leaking.
---
contrib/amcheck/verify_heapam.c | 16 +++++++----
src/backend/access/transam/parallel.c | 4 ++-
src/backend/executor/execReplication.c | 6 ++--
src/backend/executor/spi.c | 3 +-
src/backend/storage/lmgr/predicate.c | 7 +++--
src/backend/tcop/pquery.c | 24 ++++++++--------
src/backend/utils/adt/ri_triggers.c | 9 ++++--
src/backend/utils/time/snapmgr.c | 39 +++++++++++++++++++++++++-
8 files changed, 81 insertions(+), 27 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index e16557aca36..b0412e3064e 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -231,6 +231,7 @@ verify_heapam(PG_FUNCTION_ARGS)
BlockNumber last_block;
BlockNumber nblocks;
const char *skip;
+ Snapshot snapshot;
/* Check supplied arguments */
if (PG_ARGISNULL(0))
@@ -272,12 +273,6 @@ verify_heapam(PG_FUNCTION_ARGS)
ctx.cached_xid = InvalidTransactionId;
ctx.toasted_attributes = NIL;
- /*
- * Any xmin newer than the xmin of our snapshot can't become all-visible
- * while we're running.
- */
- ctx.safe_xmin = GetTransactionSnapshot()->xmin;
-
/*
* If we report corruption when not examining some individual attribute,
* we need attnum to be reported as NULL. Set that up before any
@@ -338,6 +333,13 @@ verify_heapam(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
+ /*
+ * Any xmin newer than the xmin of our snapshot can't become all-visible
+ * while we're running.
+ */
+ snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ ctx.safe_xmin = snapshot->xmin;
+
ctx.bstrategy = GetAccessStrategy(BAS_BULKREAD);
ctx.buffer = InvalidBuffer;
ctx.page = NULL;
@@ -802,6 +804,8 @@ verify_heapam(PG_FUNCTION_ARGS)
if (vmbuffer != InvalidBuffer)
ReleaseBuffer(vmbuffer);
+ UnregisterSnapshot(snapshot);
+
/* Close the associated toast table and indexes, if any. */
if (ctx.toast_indexes)
toast_close_indexes(ctx.toast_indexes, ctx.num_toast_indexes,
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 0a1e089ec1d..7d6d9636e7e 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -222,7 +222,7 @@ InitializeParallelDSM(ParallelContext *pcxt)
int i;
FixedParallelState *fps;
dsm_handle session_dsm_handle = DSM_HANDLE_INVALID;
- Snapshot transaction_snapshot = GetTransactionSnapshot();
+ Snapshot transaction_snapshot = RegisterSnapshot(GetTransactionSnapshot());
Snapshot active_snapshot = GetActiveSnapshot();
/* We might be running in a very short-lived memory context. */
@@ -494,6 +494,8 @@ InitializeParallelDSM(ParallelContext *pcxt)
/* Restore previous memory context. */
MemoryContextSwitchTo(oldcontext);
+
+ UnregisterSnapshot(transaction_snapshot);
}
/*
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 68deea50f66..5a1efe9c3a7 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -285,7 +285,7 @@ retry:
PushActiveSnapshot(GetLatestSnapshot());
- res = table_tuple_lock(rel, &(outslot->tts_tid), GetLatestSnapshot(),
+ res = table_tuple_lock(rel, &(outslot->tts_tid), GetActiveSnapshot(),
outslot,
GetCurrentCommandId(false),
lockmode,
@@ -443,7 +443,7 @@ retry:
PushActiveSnapshot(GetLatestSnapshot());
- res = table_tuple_lock(rel, &(outslot->tts_tid), GetLatestSnapshot(),
+ res = table_tuple_lock(rel, &(outslot->tts_tid), GetActiveSnapshot(),
outslot,
GetCurrentCommandId(false),
lockmode,
@@ -500,7 +500,7 @@ retry:
PushActiveSnapshot(GetLatestSnapshot());
- res = table_tuple_lock(rel, &conflictTid, GetLatestSnapshot(),
+ res = table_tuple_lock(rel, &conflictTid, GetActiveSnapshot(),
*conflictslot,
GetCurrentCommandId(false),
LockTupleShare,
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index c1d8fd08c6c..0cc83de5870 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1752,7 +1752,8 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
else
{
CommandCounterIncrement();
- snapshot = GetTransactionSnapshot();
+ /* let PortalStart call GetTransactionSnapshot() */
+ snapshot = InvalidSnapshot;
}
/*
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 2030322f957..a365a4359d2 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -3950,6 +3950,8 @@ ReleaseOneSerializableXact(SERIALIZABLEXACT *sxact, bool partial,
LWLockRelease(SerializableXactHashLock);
}
+extern List *dangling_snapshots;
+
/*
* Tests whether the given top level transaction is concurrent with
* (overlaps) our current transaction.
@@ -3967,6 +3969,7 @@ XidIsConcurrent(TransactionId xid)
Assert(!TransactionIdEquals(xid, GetTopTransactionIdIfAny()));
snap = GetTransactionSnapshot();
+ dangling_snapshots = list_delete_ptr(dangling_snapshots, snap);
if (TransactionIdPrecedes(xid, snap->xmin))
return false;
@@ -4214,7 +4217,7 @@ CheckTargetForConflictsIn(PREDICATELOCKTARGETTAG *targettag)
}
else if (!SxactIsDoomed(sxact)
&& (!SxactIsCommitted(sxact)
- || TransactionIdPrecedes(GetTransactionSnapshot()->xmin,
+ || TransactionIdPrecedes(TransactionXmin,
sxact->finishedBefore))
&& !RWConflictExists(sxact, MySerializableXact))
{
@@ -4227,7 +4230,7 @@ CheckTargetForConflictsIn(PREDICATELOCKTARGETTAG *targettag)
*/
if (!SxactIsDoomed(sxact)
&& (!SxactIsCommitted(sxact)
- || TransactionIdPrecedes(GetTransactionSnapshot()->xmin,
+ || TransactionIdPrecedes(TransactionXmin,
sxact->finishedBefore))
&& !RWConflictExists(sxact, MySerializableXact))
{
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 89d704df8d1..4ce8417f63b 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -1242,18 +1242,20 @@ PortalRunMulti(Portal portal,
{
snapshot = RegisterSnapshot(snapshot);
portal->holdSnapshot = snapshot;
- }
- /*
- * We can't have the holdSnapshot also be the active one,
- * because UpdateActiveSnapshotCommandId would complain. So
- * force an extra snapshot copy. Plain PushActiveSnapshot
- * would have copied the transaction snapshot anyway, so this
- * only adds a copy step when setHoldSnapshot is true. (It's
- * okay for the command ID of the active snapshot to diverge
- * from what holdSnapshot has.)
- */
- PushCopiedSnapshot(snapshot);
+ /* XXX
+ * We can't have the holdSnapshot also be the active one,
+ * because UpdateActiveSnapshotCommandId would complain. So
+ * force an extra snapshot copy. Plain PushActiveSnapshot
+ * would have copied the transaction snapshot anyway, so this
+ * only adds a copy step when setHoldSnapshot is true. (It's
+ * okay for the command ID of the active snapshot to diverge
+ * from what holdSnapshot has.)
+ */
+ PushCopiedSnapshot(snapshot);
+ }
+ else
+ PushActiveSnapshot(snapshot);
/*
* As for PORTAL_ONE_SELECT portals, it does not seem
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 3185f48afa6..bb6fa20ee03 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2465,8 +2465,8 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
if (IsolationUsesXactSnapshot() && detectNewRows)
{
CommandCounterIncrement(); /* be sure all my own work is visible */
- test_snapshot = GetLatestSnapshot();
- crosscheck_snapshot = GetTransactionSnapshot();
+ test_snapshot = RegisterSnapshot(GetLatestSnapshot());
+ crosscheck_snapshot = RegisterSnapshot(GetTransactionSnapshot());
}
else
{
@@ -2495,6 +2495,11 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
test_snapshot, crosscheck_snapshot,
false, false, limit);
+ if (test_snapshot != NULL)
+ UnregisterSnapshot(test_snapshot);
+ if (crosscheck_snapshot != NULL)
+ UnregisterSnapshot(crosscheck_snapshot);
+
/* Restore UID and security context */
SetUserIdAndSecContext(save_userid, save_sec_context);
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 05f16666192..f4831ed989c 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -254,6 +254,20 @@ typedef struct SerializedSnapshotData
CommandId curcid;
} SerializedSnapshotData;
+List *dangling_snapshots = NIL;
+
+static inline Snapshot
+add_dangling(Snapshot snapshot)
+{
+ MemoryContext save_cxt = CurrentMemoryContext;
+
+ MemoryContextSwitchTo(TopMemoryContext);
+ dangling_snapshots = lappend(dangling_snapshots, snapshot);
+ MemoryContextSwitchTo(save_cxt);
+ return snapshot;
+}
+
+
/*
* GetTransactionSnapshot
* Get the appropriate snapshot for a new query in a transaction.
@@ -275,6 +289,7 @@ GetTransactionSnapshot(void)
if (HistoricSnapshotActive())
{
Assert(!FirstSnapshotSet);
+ add_dangling(HistoricSnapshot);
return HistoricSnapshot;
}
@@ -319,17 +334,22 @@ GetTransactionSnapshot(void)
CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
FirstSnapshotSet = true;
+ add_dangling(CurrentSnapshot);
return CurrentSnapshot;
}
if (IsolationUsesXactSnapshot())
+ {
+ add_dangling(CurrentSnapshot);
return CurrentSnapshot;
+ }
/* Don't allow catalog snapshot to be older than xact snapshot. */
InvalidateCatalogSnapshot();
CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
+ add_dangling(CurrentSnapshot);
return CurrentSnapshot;
}
@@ -357,10 +377,12 @@ GetLatestSnapshot(void)
/* If first call in transaction, go ahead and set the xact snapshot */
if (!FirstSnapshotSet)
+ {
return GetTransactionSnapshot();
+ }
SecondarySnapshot = GetSnapshotData(&SecondarySnapshotData);
-
+ add_dangling(SecondarySnapshot);
return SecondarySnapshot;
}
@@ -380,7 +402,10 @@ GetCatalogSnapshot(Oid relid)
* finishing decoding.
*/
if (HistoricSnapshotActive())
+ {
+ add_dangling(HistoricSnapshot);
return HistoricSnapshot;
+ }
return GetNonHistoricCatalogSnapshot(relid);
}
@@ -426,6 +451,7 @@ GetNonHistoricCatalogSnapshot(Oid relid)
pairingheap_add(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
}
+ add_dangling(CatalogSnapshot);
return CatalogSnapshot;
}
@@ -684,6 +710,8 @@ PushActiveSnapshotWithLevel(Snapshot snapshot, int snap_level)
{
ActiveSnapshotElt *newactive;
+ dangling_snapshots = list_delete_ptr(dangling_snapshots, snapshot);
+
Assert(snapshot != InvalidSnapshot);
Assert(ActiveSnapshot == NULL || snap_level >= ActiveSnapshot->as_level);
@@ -695,7 +723,9 @@ PushActiveSnapshotWithLevel(Snapshot snapshot, int snap_level)
*/
if (snapshot == CurrentSnapshot || snapshot == SecondarySnapshot ||
!snapshot->copied)
+ {
newactive->as_snap = CopySnapshot(snapshot);
+ }
else
newactive->as_snap = snapshot;
@@ -828,6 +858,8 @@ RegisterSnapshotOnOwner(Snapshot snapshot, ResourceOwner owner)
if (snapshot == InvalidSnapshot)
return InvalidSnapshot;
+ dangling_snapshots = list_delete_ptr(dangling_snapshots, snapshot);
+
/* Static snapshot? Create a persistent copy */
snap = snapshot->copied ? snapshot : CopySnapshot(snapshot);
@@ -1019,6 +1051,11 @@ AtEOXact_Snapshot(bool isCommit, bool resetXmin)
}
FirstXactSnapshot = NULL;
+ foreach_ptr (Snapshot, snapshot, dangling_snapshots)
+ {
+ elog(PANIC, "had a dangling snapshot %p", snapshot);
+ }
+
/*
* If we exported any snapshots, clean them up.
*/
--
2.39.5
