Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRENTLY.

tglsfdc · tglsfdc · commit e4a55378f726 · 2024-06-17T14:30:59.000-04:00
Reconstruction of an SP-GiST index by REINDEX CONCURRENTLY may insert some REDIRECT tuples. This will typically happen in a transaction that lacks an XID, which leads either to assertion failure in spgFormDeadTuple or to insertion of a REDIRECT tuple with zero xid. The latter's not good either, since eventually VACUUM will apply GlobalVisTestIsRemovableXid() to the zero xid, resulting in either an assertion failure or a garbage answer. In practice, since REINDEX CONCURRENTLY locks out index scans till it's done, it doesn't matter whether it inserts REDIRECTs or PLACEHOLDERs; and likewise it doesn't matter how soon VACUUM reduces such a REDIRECT to a PLACEHOLDER. So in non-assert builds there's no observable problem here, other than perhaps a little index bloat. But it's not behaving as intended. To fix, remove the failing Assert in spgFormDeadTuple, acknowledging that we might sometimes insert a zero XID; and guard VACUUM's GlobalVisTestIsRemovableXid() call with a test for valid XID, ensuring that we'll reduce such a REDIRECT the first time VACUUM sees it. (Versions before v14 use TransactionIdPrecedes here, which won't fail on zero xid, so they really have no bug at all in non-assert builds.) Another solution could be to not create REDIRECTs at all during REINDEX CONCURRENTLY, making the relevant code paths treat that case like index build (which likewise knows that no concurrent index scans can be happening). That would allow restoring the Assert in spgFormDeadTuple, but we'd still need the VACUUM change because redirection tuples with zero xid may be out there already. But there doesn't seem to be a nice way for spginsert() to tell that it's being called in REINDEX CONCURRENTLY without some API changes, so we'll leave that as a possible future improvement. In HEAD, also rename the SpGistState.myXid field to redirectXid, which seems less misleading (since it might not in fact be our transaction's XID) and is certainly less uninformatively generic. Per bug #18499 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/18499-8a519c280f956480@postgresql.org
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
@@ -355,7 +355,18 @@ initSpGistState(SpGistState *state, Relation index)
 	/* Make workspace for constructing dead tuples */
 	state->deadTupleStorage = palloc0(SGDTSIZE);
 
-	/* Set XID to use in redirection tuples */
+	/*
+	 * Set horizon XID to use in redirection tuples.  Use our own XID if we
+	 * have one, else use InvalidTransactionId.  The latter case can happen in
+	 * VACUUM or REINDEX CONCURRENTLY, and in neither case would it be okay to
+	 * force an XID to be assigned.  VACUUM won't create any redirection
+	 * tuples anyway, but REINDEX CONCURRENTLY can.  Fortunately, REINDEX
+	 * CONCURRENTLY doesn't mark the index valid until the end, so there could
+	 * never be any concurrent scans "in flight" to a redirection tuple it has
+	 * inserted.  And it locks out VACUUM until the end, too.  So it's okay
+	 * for VACUUM to immediately expire a redirection tuple that contains an
+	 * invalid xid.
+	 */
 	state->myXid = GetTopTransactionIdIfAny();
 
 	/* Assume we're not in an index build (spgbuild will override) */
@@ -1082,7 +1093,6 @@ 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
@@ -189,7 +189,9 @@ vacuumLeafPage(spgBulkDeleteState *bds, Relation index, Buffer buffer,
 
 			/*
 			 * Add target TID to pending list if the redirection could have
-			 * happened since VACUUM started.
+			 * happened since VACUUM started.  (If xid is invalid, assume it
+			 * must have happened before VACUUM started, since REINDEX
+			 * CONCURRENTLY locks out VACUUM.)
 			 *
 			 * Note: we could make a tighter test by seeing if the xid is
 			 * "running" according to the active snapshot; but snapmgr.c
@@ -524,8 +526,17 @@ vacuumRedirectAndPlaceholder(Relation index, Buffer buffer)
 
 		dt = (SpGistDeadTuple) PageGetItem(page, PageGetItemId(page, i));
 
+		/*
+		 * We can convert a REDIRECT to a PLACEHOLDER if there could no longer
+		 * be any index scans "in flight" to it.  Such an index scan would
+		 * have to be in a transaction whose snapshot sees the REDIRECT's XID
+		 * as still running, so comparing the XID against global xmin is a
+		 * conservatively safe test.  If the XID is invalid, it must have been
+		 * inserted by REINDEX CONCURRENTLY, so we can zap it immediately.
+		 */
 		if (dt->tupstate == SPGIST_REDIRECT &&
-			GlobalVisTestIsRemovableXid(vistest, dt->xid))
+			(!TransactionIdIsValid(dt->xid) ||
+			 GlobalVisTestIsRemovableXid(vistest, dt->xid)))
 		{
 			dt->tupstate = SPGIST_PLACEHOLDER;
 			Assert(opaque->nRedirection > 0);
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h
@@ -421,7 +421,8 @@ typedef struct SpGistLeafTupleData
  * field, to satisfy some Asserts that we make when replacing a leaf tuple
  * with a dead tuple.
  * We don't use t_info, but it's needed to align the pointer field.
- * pointer and xid are only valid when tupstate = REDIRECT.
+ * pointer and xid are only valid when tupstate = REDIRECT, and in some
+ * cases xid can be InvalidTransactionId even then; see initSpGistState.
  */
 typedef struct SpGistDeadTupleData
 {
