Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit8076c00

Browse files
committed
Assert that a snapshot is active or registered before it's used
The comment in GetTransactionSnapshot() said that you "should callRegisterSnapshot or PushActiveSnapshot on the returned snap if it isto be used very long". That felt too unclear to me. Make the commentmore strongly worded.To enforce that rule and to catch potential bugs where a snapshotmight get invalidated while it's still in use, add an assertion toHeapTupleSatisfiesMVCC() to check that the snapshot is registered orpushed to active stack. No new bugs were found by this, but it seemslike good future-proofing. It's not a great place for the check;HeapTupleSatisfiesMVCC() is in fact safe to call with an unregisteredsnapshot, and the assertion won't catch other unsafe uses. But it goesa long way in practice.Fix a few cases that were playing fast and loose with that and justassumed that the snapshot cannot be invalidated during a scan. Thoseassumptions were not wrong, but they're not performance critical, solet's drop the excuses and just register the snapshot. These werefalse positives found by the new assertion.Discussion:https://www.postgresql.org/message-id/7c56f180-b9e1-481e-8c1d-efa63de3ecbb@iki.fi
1 parentbd65cb3 commit8076c00

File tree

5 files changed

+26
-17
lines changed

5 files changed

+26
-17
lines changed

‎src/backend/access/heap/heapam_visibility.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -962,6 +962,15 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
962962
{
963963
HeapTupleHeadertuple=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

Lines changed: 2 additions & 6 deletions
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

Lines changed: 2 additions & 1 deletion
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

Lines changed: 9 additions & 6 deletions
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
returnpg_class_tuple;

‎src/backend/utils/time/snapmgr.c

Lines changed: 4 additions & 4 deletions
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 valuemay point at static storage that will be modified
207-
* by future calls and by CommandCounterIncrement(). Callersshould call
208-
* RegisterSnapshot or PushActiveSnapshot on the returned snapif it is to be
209-
*used very long.
206+
* Note that the return valuepoints at static storage that will be modified
207+
* by future calls and by CommandCounterIncrement(). Callersmust call
208+
* RegisterSnapshot or PushActiveSnapshot on the returned snapbefore doing
209+
*any other non-trivial work that could invalidate it.
210210
*/
211211
Snapshot
212212
GetTransactionSnapshot(void)

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp