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 0e499598a428c8859168f8665662049797109f10..2ea19d268316674ed6a3e4775e71e72af6b72372 100644 (file)
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -566,7 +566,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));
 
        /*
@@ -816,7 +817,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/gistvacuum.c b/src/backend/access/gist/gistvacuum.c
index 2337dbd7f9d71e6fd51bdf94643dae67c2e1ea8d..1c4e2c19b76d5bb6096bde8a516565f0ef7a277d 100644 (file)
--- a/src/backend/access/gist/gistvacuum.c
+++ b/src/backend/access/gist/gistvacuum.c
@@ -257,7 +257,7 @@ gistbulkdelete(PG_FUNCTION_ARGS)
 
                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 3bdbe757aeb3d173f8000d86d97a1d877f79225f..527efdfb3156dde6aaed8d6de24be6ebc1d4c048 100644 (file)
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -1157,7 +1157,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 ce468af41cebbbbae85b51988e011899b118d944..ef889b9e5bc36c237eef22eef645bc7110be7f60 100644 (file)
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -1770,7 +1770,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
        {