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 f7f44b49aa30d0e2ae54b349d95ea6c2edd5f613..891ad3822e1bdbf0a77eb3c9b6306946cb1bafcd 100644 (file)
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -614,7 +614,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));
 
                /*
@@ -864,7 +865,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 c1b05d66d2937e0c9fef47a7f094c00e007a4d43..e633fec09d0b6c2e3059e004aab1059eaece1823 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 53e5cea580fd1a25fbe451661bb05708c7dcaf5c..a6369882febdfc46249da00a6a3def9ce5ae01ed 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 ee46023c5a66a65735f1df38e0d20fddd0019bae..28d0e6e7734ec894d092276cb51a5dee6d2d123d 100644 (file)
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -1168,7 +1168,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 063c988dc1f8b53b8766b26dd4ef20c2b3d9c6a2..88cf9b59c9f85c451495f3af21a7ac96fc41ac70 100644 (file)
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -1768,7 +1768,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
                {