From e2bbc273a17e8effa8366764cfc1bea5625b7f1d Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 28 Aug 2017 10:36:31 -0700 Subject: [RFC PATCH] PageGetLSN: assert that locks are properly held When accessing a shared page, callers must either hold an exclusive lock on the buffer, OR a shared lock plus the buffer header spinlock (this additional constraint exists because it's legal to write the LSN without holding an exclusive lock). Several callers are currently not following this contract. To help developers find issues, assert that the proper locks are held for shared pages inside PageGetLSN. The implementation for this check is added in a helper function, AssertPageIsLockedForLSN, which is called from PageGetLSN whenever backend code is built with assertions enabled. This introduces a slight source incompatibility: previously, both Pages and PageHeaders could be passed to the macro, but now callers must explicitly pass a Page. In places where PageGetLSN is clearly insufficient, move to BufferGetLSNAtomic instead. This is a work in progress. --- contrib/pageinspect/rawpage.c | 2 +- src/backend/access/gist/gist.c | 4 ++-- src/backend/access/gist/gistget.c | 4 ++-- src/backend/access/gist/gistvacuum.c | 2 +- src/backend/access/nbtree/nbtsearch.c | 2 +- src/backend/access/nbtree/nbtutils.c | 2 +- src/backend/storage/buffer/bufmgr.c | 25 +++++++++++++++++++++++++ src/include/storage/bufpage.h | 17 +++++++++++++++-- 8 files changed, 48 insertions(+), 10 deletions(-) diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c index 25af22f453..e3a1ca0e3b 100644 --- a/contrib/pageinspect/rawpage.c +++ b/contrib/pageinspect/rawpage.c @@ -250,7 +250,7 @@ page_header(PG_FUNCTION_ARGS) /* Extract information from the page header */ - lsn = PageGetLSN(page); + lsn = PageGetLSN((Page) page); /* pageinspect >= 1.2 uses pg_lsn instead of text for the LSN field. */ if (TupleDescAttr(tupdesc, 0)->atttypid == TEXTOID) diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c index 565525bbdf..281d2b2a16 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -640,7 +640,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate) } stack->page = (Page) BufferGetPage(stack->buffer); - stack->lsn = PageGetLSN(stack->page); + stack->lsn = BufferGetLSNAtomic(stack->buffer); Assert(!RelationNeedsWAL(state.r) || !XLogRecPtrIsInvalid(stack->lsn)); /* @@ -890,7 +890,7 @@ gistFindPath(Relation r, BlockNumber child, OffsetNumber *downlinkoffnum) break; } - top->lsn = PageGetLSN(page); + top->lsn = BufferGetLSNAtomic(buffer); /* * If F_FOLLOW_RIGHT is set, the page to the right doesn't have a diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c index 760ea0c997..68711bfc09 100644 --- a/src/backend/access/gist/gistget.c +++ b/src/backend/access/gist/gistget.c @@ -61,7 +61,7 @@ gistkillitems(IndexScanDesc scan) * read. killedItems could be not valid so LP_DEAD hints applying is not * safe. */ - if (PageGetLSN(page) != so->curPageLSN) + if (BufferGetLSNAtomic(buffer) != so->curPageLSN) { UnlockReleaseBuffer(buffer); so->numKilled = 0; /* reset counter */ @@ -384,7 +384,7 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem, double *myDistances, * safe to apply LP_DEAD hints to the page later. This allows us to drop * the pin for MVCC scans, which allows vacuum to avoid blocking. */ - so->curPageLSN = PageGetLSN(page); + so->curPageLSN = BufferGetLSNAtomic(buffer); /* * check all tuples on page diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c index 77d9d12f0b..54a61cc427 100644 --- a/src/backend/access/gist/gistvacuum.c +++ b/src/backend/access/gist/gistvacuum.c @@ -249,7 +249,7 @@ gistbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats, ptr = (GistBDItem *) palloc(sizeof(GistBDItem)); ptr->blkno = ItemPointerGetBlockNumber(&(idxtuple->t_tid)); - ptr->parentlsn = PageGetLSN(page); + ptr->parentlsn = BufferGetLSNAtomic(buffer); ptr->next = stack->next; stack->next = ptr; diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c index 642c8943e7..1ca2623dea 100644 --- a/src/backend/access/nbtree/nbtsearch.c +++ b/src/backend/access/nbtree/nbtsearch.c @@ -1224,7 +1224,7 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum) * safe to apply LP_DEAD hints to the page later. This allows us to drop * the pin for MVCC scans, which allows vacuum to avoid blocking. */ - so->currPos.lsn = PageGetLSN(page); + so->currPos.lsn = BufferGetLSNAtomic(so->currPos.buf); /* * we must save the page's right-link while scanning it; this tells us diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c index dbfb775dec..4360661789 100644 --- a/src/backend/access/nbtree/nbtutils.c +++ b/src/backend/access/nbtree/nbtutils.c @@ -1772,7 +1772,7 @@ _bt_killitems(IndexScanDesc scan) return; page = BufferGetPage(buf); - if (PageGetLSN(page) == so->currPos.lsn) + if (BufferGetLSNAtomic(buf) == so->currPos.lsn) so->currPos.buf = buf; else { diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 15795b0c5a..b871c76db7 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -4355,3 +4355,28 @@ TestForOldSnapshot_impl(Snapshot snapshot, Relation relation) (errcode(ERRCODE_SNAPSHOT_TOO_OLD), errmsg("snapshot too old"))); } + +#if defined(USE_ASSERT_CHECKING) && !defined(FRONTEND) +void +AssertPageIsLockedForLSN(Page page) +{ + char *pagePtr = page; + + /* + * We only want to assert that we hold a lock on the page contents if the + * page is shared (i.e. it is one of the BufferBlocks). + */ + if (BufferBlocks <= pagePtr + && pagePtr < (BufferBlocks + NBuffers * BLCKSZ)) + { + ptrdiff_t bufId = (pagePtr - BufferBlocks) / BLCKSZ; + BufferDesc *buf = GetBufferDescriptor(bufId); + LWLock *content_lock = BufferDescriptorGetContentLock(buf); + uint32 buf_state = pg_atomic_read_u32(&buf->state); + + Assert(LWLockHeldByMeInMode(content_lock, LW_EXCLUSIVE) + || (LWLockHeldByMeInMode(content_lock, LW_SHARED) + && (buf_state & BM_LOCKED))); + } +} +#endif diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h index 50c72a3c8d..db94310586 100644 --- a/src/include/storage/bufpage.h +++ b/src/include/storage/bufpage.h @@ -359,8 +359,21 @@ PageValidateSpecialPointer(Page page) * Additional macros for access to page headers. (Beware multiple evaluation * of the arguments!) */ -#define PageGetLSN(page) \ - PageXLogRecPtrGet(((PageHeader) (page))->pd_lsn) + +#if defined(USE_ASSERT_CHECKING) && !defined(FRONTEND) +/* in bufmgr.c; used in PageGetLSN */ +extern void AssertPageIsLockedForLSN(Page page); +#else +#define AssertPageIsLockedForLSN(page) +#endif + +static inline XLogRecPtr +PageGetLSN(Page page) +{ + AssertPageIsLockedForLSN(page); + return PageXLogRecPtrGet(((PageHeader) page)->pd_lsn); +} + #define PageSetLSN(page, lsn) \ PageXLogRecPtrSet(((PageHeader) (page))->pd_lsn, lsn) -- 2.14.0