From 272c2ab9fd0a604e3200030b1ea26fd464c44935 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Tue, 9 Jan 2018 15:54:39 -0300 Subject: [PATCH] Change some bogus PageGetLSN calls to BufferGetLSNAtomic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As src/backend/access/transam/README says, PageGetLSN may only be called by processes holding either exclusive lock on buffer, or a shared lock on buffer plus buffer header lock. Therefore any place that only holds a shared buffer lock must use BufferGetLSNAtomic instead of PageGetLSN, which internally obtains buffer header lock prior to reading the LSN. A few callsites failed to comply with this rule. This was detected by running all tests under a new (not committed) assertion that verifies PageGetLSN locking contract. All but one of the callsites that failed the assertion are fixed by this patch. Remaining callsites were inspected manually and determined not to need any change. The exception (unfixed callsite) is in TestForOldSnapshot, which only has a Page argument, making it impossible to access the corresponding Buffer from it. Fixing that seems a much larger patch that will have to be done separately; and that's just as well, since it was only introduced in 9.6 and other bugs are much older. Some of these bugs are ancient; backpatch all the way back to 9.3. Authors: Jacob Champion, Asim Praveen, Ashwin Agrawal Reviewed-by: Michaël Paquier Discussion: https://postgr.es/m/CABAq_6GXgQDVu3u12mK9O5Xt5abBZWQ0V40LZCE+oUf95XyNFg@mail.gmail.com --- src/backend/access/gist/gist.c | 5 +++-- 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 +- 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c index aff969ead4..51c32e4afe 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -640,7 +640,8 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate) } stack->page = (Page) BufferGetPage(stack->buffer); - stack->lsn = PageGetLSN(stack->page); + stack->lsn = xlocked ? + PageGetLSN(stack->page) : BufferGetLSNAtomic(stack->buffer); Assert(!RelationNeedsWAL(state.r) || !XLogRecPtrIsInvalid(stack->lsn)); /* @@ -890,7 +891,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 ca21cf7047..b30b931c3b 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 95a0c54f63..22181c6299 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 847434fec6..51dca64e13 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 c62e4ef782..752667c885 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 {