Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 8076c00

Browse files
committed
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
1 parent bd65cb3 commit 8076c00

File tree

5 files changed

+26
-17
lines changed

5 files changed

+26
-17
lines changed

src/backend/access/heap/heapam_visibility.c

+9
Original file line numberDiff line numberDiff line change
@@ -962,6 +962,15 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
962962
{
963963
HeapTupleHeader tuple = htup->t_data;
964964

965+
/*
966+
* Assert that the caller has registered the snapshot. This function
967+
* doesn't care about the registration as such, but in general you
968+
* shouldn't try to use a snapshot without registration because it might
969+
* get invalidated while it's still in use, and this is a convenient place
970+
* to check for that.
971+
*/
972+
Assert(snapshot->regd_count > 0 || snapshot->active_count > 0);
973+
965974
Assert(ItemPointerIsValid(&htup->t_self));
966975
Assert(htup->t_tableOid != InvalidOid);
967976

src/backend/access/index/genam.c

+2-6
Original file line numberDiff line numberDiff line change
@@ -577,17 +577,13 @@ systable_recheck_tuple(SysScanDesc sysscan, HeapTuple tup)
577577

578578
Assert(tup == ExecFetchSlotHeapTuple(sysscan->slot, false, NULL));
579579

580-
/*
581-
* Trust that table_tuple_satisfies_snapshot() and its subsidiaries
582-
* (commonly LockBuffer() and HeapTupleSatisfiesMVCC()) do not themselves
583-
* acquire snapshots, so we need not register the snapshot. Those
584-
* facilities are too low-level to have any business scanning tables.
585-
*/
586580
freshsnap = GetCatalogSnapshot(RelationGetRelid(sysscan->heap_rel));
581+
freshsnap = RegisterSnapshot(freshsnap);
587582

588583
result = table_tuple_satisfies_snapshot(sysscan->heap_rel,
589584
sysscan->slot,
590585
freshsnap);
586+
UnregisterSnapshot(freshsnap);
591587

592588
/*
593589
* Handle the concurrent abort while fetching the catalog tuple during

src/backend/commands/dbcommands.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,7 @@ ScanSourceDatabasePgClass(Oid tbid, Oid dbid, char *srcpath)
288288
* snapshot - or the active snapshot - might not be new enough for that,
289289
* but the return value of GetLatestSnapshot() should work fine.
290290
*/
291-
snapshot = GetLatestSnapshot();
291+
snapshot = RegisterSnapshot(GetLatestSnapshot());
292292

293293
/* Process the relation block by block. */
294294
for (blkno = 0; blkno < nblocks; blkno++)
@@ -313,6 +313,7 @@ ScanSourceDatabasePgClass(Oid tbid, Oid dbid, char *srcpath)
313313

314314
UnlockReleaseBuffer(buf);
315315
}
316+
UnregisterSnapshot(snapshot);
316317

317318
/* Release relation lock. */
318319
UnlockRelationId(&relid, AccessShareLock);

src/backend/utils/cache/relcache.c

+9-6
Original file line numberDiff line numberDiff line change
@@ -371,14 +371,13 @@ ScanPgRelation(Oid targetRelId, bool indexOK, bool force_non_historic)
371371
pg_class_desc = table_open(RelationRelationId, AccessShareLock);
372372

373373
/*
374-
* The caller might need a tuple that's newer than the one the historic
375-
* snapshot; currently the only case requiring to do so is looking up the
376-
* relfilenumber of non mapped system relations during decoding. That
377-
* snapshot can't change in the midst of a relcache build, so there's no
378-
* need to register the snapshot.
374+
* The caller might need a tuple that's newer than what's visible to the
375+
* historic snapshot; currently the only case requiring to do so is
376+
* looking up the relfilenumber of non mapped system relations during
377+
* decoding.
379378
*/
380379
if (force_non_historic)
381-
snapshot = GetNonHistoricCatalogSnapshot(RelationRelationId);
380+
snapshot = RegisterSnapshot(GetNonHistoricCatalogSnapshot(RelationRelationId));
382381

383382
pg_class_scan = systable_beginscan(pg_class_desc, ClassOidIndexId,
384383
indexOK && criticalRelcachesBuilt,
@@ -395,6 +394,10 @@ ScanPgRelation(Oid targetRelId, bool indexOK, bool force_non_historic)
395394

396395
/* all done */
397396
systable_endscan(pg_class_scan);
397+
398+
if (snapshot)
399+
UnregisterSnapshot(snapshot);
400+
398401
table_close(pg_class_desc, AccessShareLock);
399402

400403
return pg_class_tuple;

src/backend/utils/time/snapmgr.c

+4-4
Original file line numberDiff line numberDiff line change
@@ -203,10 +203,10 @@ typedef struct SerializedSnapshotData
203203
* GetTransactionSnapshot
204204
* Get the appropriate snapshot for a new query in a transaction.
205205
*
206-
* Note that the return value may point at static storage that will be modified
207-
* by future calls and by CommandCounterIncrement(). Callers should call
208-
* RegisterSnapshot or PushActiveSnapshot on the returned snap if it is to be
209-
* used very long.
206+
* Note that the return value points at static storage that will be modified
207+
* by future calls and by CommandCounterIncrement(). Callers must call
208+
* RegisterSnapshot or PushActiveSnapshot on the returned snap before doing
209+
* any other non-trivial work that could invalidate it.
210210
*/
211211
Snapshot
212212
GetTransactionSnapshot(void)

0 commit comments

Comments
 (0)