Avoid early reuse of btree pages, causing incorrect query results.

When we allowed read-only transactions to skip assigning XIDs
we introduced the possibility that a fully deleted btree page
could be reused. This broke the index link sequence which could
then lead to indexscans silently returning fewer rows than would
have been correct. The actual incidence of silent errors from
this is thought to be very low because of the exact workload
required and locking pre-conditions. Fix is to remove pages only
if index page opaque->btpo.xact precedes RecentGlobalXmin.

Noah Misch, reviewed by Simon Riggs

diff --git a/src/backend/access/nbtree/README b/src/backend/access/nbtree/README
index 561ffbb9d479eec36b7b589ff7f19c456e73c0be..6932c2b54971ae7ff993ff173eefea45691de8f2 100644 (file)
--- a/src/backend/access/nbtree/README
+++ b/src/backend/access/nbtree/README
@@ -261,12 +261,14 @@ we need to be sure we don't miss or re-scan any items.
 
 A deleted page can only be reclaimed once there is no scan or search that
 has a reference to it; until then, it must stay in place with its
-right-link undisturbed.  We implement this by waiting until all
-transactions that were running at the time of deletion are dead; which is
+right-link undisturbed.  We implement this by waiting until all active
+snapshots and registered snapshots as of the deletion are gone; which is
 overly strong, but is simple to implement within Postgres.  When marked
 dead, a deleted page is labeled with the next-transaction counter value.
 VACUUM can reclaim the page for re-use when this transaction number is
-older than the oldest open transaction.
+older than RecentGlobalXmin.  As collateral damage, this implementation
+also waits for running XIDs with no snapshots and for snapshots taken
+until the next transaction to allocate an XID commits.
 
 Reclaiming a page doesn't actually change its state on disk --- we simply
 record it in the shared-memory free space map, from which it will be

diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 9e3443624434e9e2393d977dc7af388697a213b7..d782d55aa47421b20b2980afcfb26f0e51b2a6e6 100644 (file)
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -560,19 +560,9 @@ _bt_getbuf(Relation rel, BlockNumber blkno, int access)
                                         */
                                        if (XLogStandbyInfoActive())
                                        {
-                                               TransactionId latestRemovedXid;
-
                                                BTPageOpaque opaque = (BTPageOpaque) PageGetSpecialPointer(page);
 
-                                               /*
-                                                * opaque->btpo.xact is the threshold value not the
-                                                * value to measure conflicts against. We must retreat
-                                                * by one from it to get the correct conflict xid.
-                                                */
-                                               latestRemovedXid = opaque->btpo.xact;
-                                               TransactionIdRetreat(latestRemovedXid);
-
-                                               _bt_log_reuse_page(rel, blkno, latestRemovedXid);
+                                               _bt_log_reuse_page(rel, blkno, opaque->btpo.xact);
                                        }
 
                                        /* Okay to use page.  Re-initialize and return it */
@@ -687,7 +677,6 @@ bool
 _bt_page_recyclable(Page page)
 {
        BTPageOpaque opaque;
-       TransactionId cutoff;
 
        /*
         * It's possible to find an all-zeroes page in an index --- for example, a
@@ -700,18 +689,11 @@ _bt_page_recyclable(Page page)
 
        /*
         * Otherwise, recycle if deleted and too old to have any processes
-        * interested in it.  If we are generating records for Hot Standby
-        * defer page recycling until RecentGlobalXmin to respect user
-        * controls specified by vacuum_defer_cleanup_age or hot_standby_feedback.
+        * interested in it.
         */
-       if (XLogStandbyInfoActive())
-               cutoff = RecentGlobalXmin;
-       else
-               cutoff = RecentXmin;
-
        opaque = (BTPageOpaque) PageGetSpecialPointer(page);
        if (P_ISDELETED(opaque) &&
-               TransactionIdPrecedesOrEquals(opaque->btpo.xact, cutoff))
+               TransactionIdPrecedes(opaque->btpo.xact, RecentGlobalXmin))
                return true;
        return false;
 }
@@ -1364,7 +1346,13 @@ _bt_pagedel(Relation rel, Buffer buf, BTStack stack)
 
        /*
         * Mark the page itself deleted.  It can be recycled when all current
-        * transactions are gone.
+        * transactions are gone.  Storing GetTopTransactionId() would work, but
+        * we're in VACUUM and would not otherwise have an XID.  Having already
+        * updated links to the target, ReadNewTransactionId() suffices as an
+        * upper bound.  Any scan having retained a now-stale link is advertising
+        * in its PGPROC an xmin less than or equal to the value we read here.  It
+        * will continue to do so, holding back RecentGlobalXmin, for the duration
+        * of that scan.
         */
        page = BufferGetPage(buf);
        opaque = (BTPageOpaque) PageGetSpecialPointer(page);

diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index 2775ae6d292567d2c73b7ad30e63c61e6145b22c..055abf90e9e1d2f7e81e3911ac02e997083a4f55 100644 (file)
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -971,7 +971,11 @@ btree_redo(XLogRecPtr lsn, XLogRecord *record)
                                /*
                                 * Btree reuse page records exist to provide a conflict point
                                 * when we reuse pages in the index via the FSM. That's all it
-                                * does though.
+                                * does though. latestRemovedXid was the page's btpo.xact. The
+                                * btpo.xact < RecentGlobalXmin test in _bt_page_recyclable()
+                                * conceptually mirrors the pgxact->xmin > limitXmin test in
+                                * GetConflictingVirtualXIDs().  Consequently, one XID value
+                                * achieves the same exclusion effect on master and standby.
                                 */
                                {
                                        xl_btree_reuse_page *xlrec = (xl_btree_reuse_page *) XLogRecGetData(record);