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 565525bbdfceb95407137fd962f3d58bcf8fdf38..fca474509f15b0624fe90c81e9eb921c4943407b 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 760ea0c997e4e73c134ad79b7fddfc14c8b6d52a..68711bfc092a1d37afc46b25457a1d423f98278a 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 77d9d12f0b7a181ad846eaa7bd6410b203227090..54a61cc42735727eac3101d8fc8c065b620d580f 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 e48d4482d1f1deccda5d8ede1f77236818e2a7d8..0151f2c91d1897cc677d5867b6f8cef363077982 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 dbfb775dec84f5a1ffb125345672410ba791f584..4360661789e950a3665a24981aa67ae41ec9956e 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
        {