Change some bogus PageGetLSN calls to BufferGetLSNAtomic

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

diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index aff969ead438c3afffcceb8810757023a91f6089..51c32e4afeeeb3af54cfa6b93fd1cdedb8377e0c 100644 (file)
--- 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 ca21cf70479cc78676b9b0c6080ba5ceff6123d3..b30b931c3b8ec728323d5058861c4c8bbf5b52c9 100644 (file)
--- 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 95a0c54f63f6295e3be06ca5357f74854647a7b3..22181c6299bbdf448b6bae363f70d493602e15e0 100644 (file)
--- 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 847434fec6f2542e0d857d1ea265733050b758ba..51dca64e139a84946022f2169b3750ce8fb85cb2 100644 (file)
--- 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 c62e4ef7821ba423fb3aff4390a26e2e2a75cda3..752667c8856a70ce4a129b725d59aae1531adf08 100644 (file)
--- 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
                {