From 7f7c93f85f5fb7fa86c525e04f3de031f3dde794 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 2 Aug 2012 15:34:21 -0400 Subject: [PATCH] Fix race conditions associated with SPGiST redirection tuples. The correct test for whether a redirection tuple is removable is whether tuple's xid < RecentGlobalXmin, not OldestXmin; the previous coding failed to protect index searches being done in concurrent transactions that have no XID. This mirrors the recent fix in btree's page recycling logic made in commit d3abbbebe52eb1e59e621c880ad57df9d40d13f2. Also, WAL-log the newest XID of any removed redirection tuple on an index page, and apply ResolveRecoveryConflictWithSnapshot during InHotStandby WAL replay. This protects against concurrent Hot Standby transactions possibly needing to see the redirection tuple(s). Per my query of 2012-03-12 and subsequent discussion. --- src/backend/access/spgist/spgutils.c | 1 + src/backend/access/spgist/spgvacuum.c | 20 +++++++++++--------- src/backend/access/spgist/spgxlog.c | 17 ++++++++++++++--- src/include/access/spgist_private.h | 1 + 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c index afa1484100..72aae02b45 100644 --- a/src/backend/access/spgist/spgutils.c +++ b/src/backend/access/spgist/spgutils.c @@ -722,6 +722,7 @@ spgFormDeadTuple(SpGistState *state, int tupstate, if (tupstate == SPGIST_REDIRECT) { ItemPointerSet(&tuple->pointer, blkno, offnum); + Assert(TransactionIdIsValid(state->myXid)); tuple->xid = state->myXid; } else diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.c index 27b55170cb..26bbd656c1 100644 --- a/src/backend/access/spgist/spgvacuum.c +++ b/src/backend/access/spgist/spgvacuum.c @@ -24,7 +24,6 @@ #include "storage/bufmgr.h" #include "storage/indexfsm.h" #include "storage/lmgr.h" -#include "storage/procarray.h" #include "utils/snapmgr.h" @@ -49,7 +48,6 @@ typedef struct spgBulkDeleteState SpGistState spgstate; /* for SPGiST operations that need one */ spgVacPendingItem *pendingList; /* TIDs we need to (re)visit */ TransactionId myXmin; /* for detecting newly-added redirects */ - TransactionId OldestXmin; /* for deciding a redirect is obsolete */ BlockNumber lastFilledBlock; /* last non-deletable block */ } spgBulkDeleteState; @@ -491,8 +489,7 @@ vacuumLeafRoot(spgBulkDeleteState *bds, Relation index, Buffer buffer) * Unlike the routines above, this works on both leaf and inner pages. */ static void -vacuumRedirectAndPlaceholder(Relation index, Buffer buffer, - TransactionId OldestXmin) +vacuumRedirectAndPlaceholder(Relation index, Buffer buffer) { Page page = BufferGetPage(buffer); SpGistPageOpaque opaque = SpGistPageGetOpaque(page); @@ -509,6 +506,7 @@ vacuumRedirectAndPlaceholder(Relation index, Buffer buffer, xlrec.node = index->rd_node; xlrec.blkno = BufferGetBlockNumber(buffer); xlrec.nToPlaceholder = 0; + xlrec.newestRedirectXid = InvalidTransactionId; START_CRIT_SECTION(); @@ -526,13 +524,18 @@ vacuumRedirectAndPlaceholder(Relation index, Buffer buffer, dt = (SpGistDeadTuple) PageGetItem(page, PageGetItemId(page, i)); if (dt->tupstate == SPGIST_REDIRECT && - TransactionIdPrecedes(dt->xid, OldestXmin)) + TransactionIdPrecedes(dt->xid, RecentGlobalXmin)) { dt->tupstate = SPGIST_PLACEHOLDER; Assert(opaque->nRedirection > 0); opaque->nRedirection--; opaque->nPlaceholder++; + /* remember newest XID among the removed redirects */ + if (!TransactionIdIsValid(xlrec.newestRedirectXid) || + TransactionIdPrecedes(xlrec.newestRedirectXid, dt->xid)) + xlrec.newestRedirectXid = dt->xid; + ItemPointerSetInvalid(&dt->pointer); itemToPlaceholder[xlrec.nToPlaceholder] = i; @@ -640,13 +643,13 @@ spgvacuumpage(spgBulkDeleteState *bds, BlockNumber blkno) else { vacuumLeafPage(bds, index, buffer, false); - vacuumRedirectAndPlaceholder(index, buffer, bds->OldestXmin); + vacuumRedirectAndPlaceholder(index, buffer); } } else { /* inner page */ - vacuumRedirectAndPlaceholder(index, buffer, bds->OldestXmin); + vacuumRedirectAndPlaceholder(index, buffer); } /* @@ -723,7 +726,7 @@ spgprocesspending(spgBulkDeleteState *bds) /* deal with any deletable tuples */ vacuumLeafPage(bds, index, buffer, true); /* might as well do this while we are here */ - vacuumRedirectAndPlaceholder(index, buffer, bds->OldestXmin); + vacuumRedirectAndPlaceholder(index, buffer); SpGistSetLastUsedPage(index, buffer); @@ -806,7 +809,6 @@ spgvacuumscan(spgBulkDeleteState *bds) initSpGistState(&bds->spgstate, index); bds->pendingList = NULL; bds->myXmin = GetActiveSnapshot()->xmin; - bds->OldestXmin = GetOldestXmin(true, false); bds->lastFilledBlock = SPGIST_LAST_FIXED_BLKNO; /* diff --git a/src/backend/access/spgist/spgxlog.c b/src/backend/access/spgist/spgxlog.c index 82f8c8b978..45eda70ac7 100644 --- a/src/backend/access/spgist/spgxlog.c +++ b/src/backend/access/spgist/spgxlog.c @@ -15,8 +15,9 @@ #include "postgres.h" #include "access/spgist_private.h" +#include "access/transam.h" #include "access/xlogutils.h" -#include "storage/bufmgr.h" +#include "storage/standby.h" #include "utils/memutils.h" @@ -888,6 +889,15 @@ spgRedoVacuumRedirect(XLogRecPtr lsn, XLogRecord *record) ptr += sizeof(spgxlogVacuumRedirect); itemToPlaceholder = (OffsetNumber *) ptr; + /* + * If any redirection tuples are being removed, make sure there are no + * live Hot Standby transactions that might need to see them. This code + * behaves similarly to btree's XLOG_BTREE_REUSE_PAGE case. + */ + if (InHotStandby && TransactionIdIsValid(xldata->newestRedirectXid)) + ResolveRecoveryConflictWithSnapshot(xldata->newestRedirectXid, + xldata->node); + if (!(record->xl_info & XLR_BKP_BLOCK_1)) { buffer = XLogReadBuffer(xldata->node, xldata->blkno, false); @@ -1060,8 +1070,9 @@ spg_desc(StringInfo buf, uint8 xl_info, char *rec) break; case XLOG_SPGIST_VACUUM_REDIRECT: out_target(buf, ((spgxlogVacuumRedirect *) rec)->node); - appendStringInfo(buf, "vacuum redirect tuples on page %u", - ((spgxlogVacuumRedirect *) rec)->blkno); + appendStringInfo(buf, "vacuum redirect tuples on page %u, newest XID %u", + ((spgxlogVacuumRedirect *) rec)->blkno, + ((spgxlogVacuumRedirect *) rec)->newestRedirectXid); break; default: appendStringInfo(buf, "unknown spgist op code %u", info); diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h index 74267a4390..b5bc45121a 100644 --- a/src/include/access/spgist_private.h +++ b/src/include/access/spgist_private.h @@ -591,6 +591,7 @@ typedef struct spgxlogVacuumRedirect BlockNumber blkno; /* block number to clean */ uint16 nToPlaceholder; /* number of redirects to make placeholders */ OffsetNumber firstPlaceholder; /* first placeholder tuple to remove */ + TransactionId newestRedirectXid; /* newest XID of removed redirects */ /* offsets of redirect tuples to make placeholders follow */ } spgxlogVacuumRedirect; -- 2.40.0