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

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

Author: tglsfdc

src/backend/access/spgist/spgutils.c

@@ -358,8 +358,19 @@ initSpGistState(SpGistState *state, Relation index)
 	/* Make workspace for constructing dead tuples */
 	state->deadTupleStorage = palloc0(SGDTSIZE);
 
-	/* Set XID to use in redirection tuples */
-	state->myXid = GetTopTransactionIdIfAny();
+	/*
+	 * 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->redirectXid = GetTopTransactionIdIfAny();
 
 	/* Assume we're not in an index build (spgbuild will override) */
 	state->isBuild = false;
@@ -1075,8 +1086,7 @@ spgFormDeadTuple(SpGistState *state, int tupstate,
 	if (tupstate == SPGIST_REDIRECT)
 	{
 		ItemPointerSet(&tuple->pointer, blkno, offnum);
-		Assert(TransactionIdIsValid(state->myXid));
-		tuple->xid = state->myXid;
+		tuple->xid = state->redirectXid;
 	}
 	else
 	{

src/backend/access/spgist/spgvacuum.c

@@ -188,7 +188,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
@@ -523,8 +525,17 @@ vacuumRedirectAndPlaceholder(Relation index, Relation heaprel, 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);

src/backend/access/spgist/spgxlog.c

@@ -36,7 +36,7 @@ fillFakeState(SpGistState *state, spgxlogState stateSrc)
 {
 	memset(state, 0, sizeof(*state));
 
-	state->myXid = stateSrc.myXid;
+	state->redirectXid = stateSrc.redirectXid;
 	state->isBuild = stateSrc.isBuild;
 	state->deadTupleStorage = palloc0(SGDTSIZE);
 }

src/include/access/spgist_private.h

@@ -157,7 +157,7 @@ typedef struct SpGistState
 
 	char	   *deadTupleStorage;	/* workspace for spgFormDeadTuple */
 
-	TransactionId myXid;		/* XID to use when creating a redirect tuple */
+	TransactionId redirectXid;	/* XID to use when creating a redirect tuple */
 	bool		isBuild;		/* true if doing index build */
 } SpGistState;
 
@@ -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
 {
@@ -464,7 +465,7 @@ typedef SpGistDeadTupleData *SpGistDeadTuple;
 
 #define STORE_STATE(s, d)  \
 	do { \
-		(d).myXid = (s)->myXid; \
+		(d).redirectXid = (s)->redirectXid; \
 		(d).isBuild = (s)->isBuild; \
 	} while(0)
 

src/include/access/spgxlog.h

@@ -35,7 +35,7 @@
  */
 typedef struct spgxlogState
 {
-	TransactionId myXid;
+	TransactionId redirectXid;
 	bool		isBuild;
 } spgxlogState;