Assert that a snapshot is active or registered before it's used

The comment in GetTransactionSnapshot() said that you "should call
RegisterSnapshot or PushActiveSnapshot on the returned snap if it is
to be used very long". That felt too unclear to me. Make the comment
more strongly worded.

To enforce that rule and to catch potential bugs where a snapshot
might get invalidated while it's still in use, add an assertion to
HeapTupleSatisfiesMVCC() to check that the snapshot is registered or
pushed to active stack. No new bugs were found by this, but it seems
like good future-proofing. It's not a great place for the check;
HeapTupleSatisfiesMVCC() is in fact safe to call with an unregistered
snapshot, and the assertion won't catch other unsafe uses. But it goes
a long way in practice.

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. These were
false positives found by the new assertion.

Discussion: https://www.postgresql.org/message-id/7c56f180-b9e1-481e-8c1d-efa63de3ecbb@iki.fi

diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index e146605bd57e5e304cec6621f56569c4333c2593..05f6946fe60d274597735fae982d78dc9be538da 100644 (file)
--- 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 886c05655f4c6a8961fbad52dc967adcc1ae74ea..8f532e14590e7c74440274e6455ffccd9902eef8 100644 (file)
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -577,17 +577,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 1753b2890743166825669cedec6a9171d2ba1f08..5fbbcdaabb1d20edcaa15e162a3dc9bcd9e76391 100644 (file)
--- 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 d1ae761b3f6e39a43848be45941e7a8d60c85f7f..9f54a9e72b730abeaf8d8c25a2d1c1674a35638f 100644 (file)
--- 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 8f1508b1ee206cfcd2533bd3ee3f7dd00b85d64d..6ed1c93383e0a1f36310f517f1ae1f1e2fd93e99 100644 (file)
--- 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)