From 2d092016fc1aeb1dae2759f451180ea898cdebc0 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 29 Jan 2024 08:52:56 +0200
Subject: [PATCH] Remove unnecessary PageIndexTupleOverwrite calls that caused
 UB (#438)

These places called PageIndexTupleOverwrite(), with the new tuple
pointing directly to the original page. The PageIndexTupleOverwrite()
call is unnecessary in these cases, as we have already modified the
tuple on the page directly. Moreover, PageIndexTupleOverWrite() will
call memcpy with same src and dst arguments, which is undefined
behavior.

It's OK to modify the pages on disk directly, and we don't need
critical sections, because we either use the generic xlog functions
which create a temporary copy of the page, or we are building a new
index so if we crash the whole index is invisible and will be dropped
anyway.
---
 src/hnswinsert.c | 16 ++--------------
 src/hnswvacuum.c | 19 +------------------
 2 files changed, 3 insertions(+), 32 deletions(-)

diff --git a/src/hnswinsert.c b/src/hnswinsert.c
index d3d0abd..fc51528 100644
--- a/src/hnswinsert.c
+++ b/src/hnswinsert.c
@@ -357,7 +357,6 @@ HnswUpdateNeighborsOnDisk(Relation index, FmgrInfo *procinfo, Oid collation, Hns
 			GenericXLogState *state;
 			ItemId		itemid;
 			HnswNeighborTuple ntup;
-			Size		ntupSize;
 			int			idx = -1;
 			int			startIdx;
 			HnswElement neighborElement = HnswPtrAccess(base, hc->element);
@@ -398,7 +397,6 @@ HnswUpdateNeighborsOnDisk(Relation index, FmgrInfo *procinfo, Oid collation, Hns
 			/* Get tuple */
 			itemid = PageGetItemId(page, offno);
 			ntup = (HnswNeighborTuple) PageGetItem(page, itemid);
-			ntupSize = ItemIdGetLength(itemid);
 
 			/* Calculate index for update */
 			startIdx = (neighborElement->level - lc) * m;
@@ -427,13 +425,9 @@ HnswUpdateNeighborsOnDisk(Relation index, FmgrInfo *procinfo, Oid collation, Hns
 			{
 				ItemPointer indextid = &ntup->indextids[idx];
 
-				/* Update neighbor */
+				/* Update neighbor on the buffer */
 				ItemPointerSet(indextid, e->blkno, e->offno);
 
-				/* Overwrite tuple */
-				if (!PageIndexTupleOverwrite(page, offno, (Item) ntup, ntupSize))
-					elog(ERROR, "failed to add index item to \"%s\"", RelationGetRelationName(index));
-
 				/* Commit */
 				if (building)
 					MarkBufferDirty(buf);
@@ -459,7 +453,6 @@ AddDuplicateOnDisk(Relation index, HnswElement element, HnswElement dup, bool bu
 	GenericXLogState *state;
 	ItemId		itemid;
 	HnswElementTuple etup;
-	Size		etupSize;
 	int			i;
 
 	/* Read page */
@@ -479,7 +472,6 @@ AddDuplicateOnDisk(Relation index, HnswElement element, HnswElement dup, bool bu
 	/* Find space */
 	itemid = PageGetItemId(page, dup->offno);
 	etup = (HnswElementTuple) PageGetItem(page, itemid);
-	etupSize = ItemIdGetLength(itemid);
 	for (i = 0; i < HNSW_HEAPTIDS; i++)
 	{
 		if (!ItemPointerIsValid(&etup->heaptids[i]))
@@ -495,13 +487,9 @@ AddDuplicateOnDisk(Relation index, HnswElement element, HnswElement dup, bool bu
 		return false;
 	}
 
-	/* Add heap TID */
+	/* Add heap TID, modifying the tuple on the page directly */
 	etup->heaptids[i] = element->heaptids[0];
 
-	/* Overwrite tuple */
-	if (!PageIndexTupleOverwrite(page, dup->offno, (Item) etup, etupSize))
-		elog(ERROR, "failed to add index item to \"%s\"", RelationGetRelationName(index));
-
 	/* Commit */
 	if (building)
 		MarkBufferDirty(buf);
diff --git a/src/hnswvacuum.c b/src/hnswvacuum.c
index b530cdb..53b2a18 100644
--- a/src/hnswvacuum.c
+++ b/src/hnswvacuum.c
@@ -92,15 +92,10 @@ RemoveHeapTids(HnswVacuumState * vacuumstate)
 
 				if (itemUpdated)
 				{
-					Size		etupSize = ItemIdGetLength(itemid);
-
 					/* Mark rest as invalid */
 					for (int i = idx; i < HNSW_HEAPTIDS; i++)
 						ItemPointerSetInvalid(&etup->heaptids[i]);
 
-					if (!PageIndexTupleOverwrite(page, offno, (Item) etup, etupSize))
-						elog(ERROR, "failed to add index item to \"%s\"", RelationGetRelationName(index));
-
 					updated = true;
 				}
 			}
@@ -482,8 +477,6 @@ MarkDeleted(HnswVacuumState * vacuumstate)
 			ItemId		itemid = PageGetItemId(page, offno);
 			HnswElementTuple etup = (HnswElementTuple) PageGetItem(page, itemid);
 			HnswNeighborTuple ntup;
-			Size		etupSize;
-			Size		ntupSize;
 			Buffer		nbuf;
 			Page		npage;
 			BlockNumber neighborPage;
@@ -507,10 +500,6 @@ MarkDeleted(HnswVacuumState * vacuumstate)
 			if (ItemPointerIsValid(&etup->heaptids[0]))
 				continue;
 
-			/* Calculate sizes */
-			etupSize = ItemIdGetLength(itemid);
-			ntupSize = HNSW_NEIGHBOR_TUPLE_SIZE(etup->level, vacuumstate->m);
-
 			/* Get neighbor page */
 			neighborPage = ItemPointerGetBlockNumber(&etup->neighbortid);
 			neighborOffno = ItemPointerGetOffsetNumber(&etup->neighbortid);
@@ -537,13 +526,7 @@ MarkDeleted(HnswVacuumState * vacuumstate)
 			for (int i = 0; i < ntup->count; i++)
 				ItemPointerSetInvalid(&ntup->indextids[i]);
 
-			/* Overwrite element tuple */
-			if (!PageIndexTupleOverwrite(page, offno, (Item) etup, etupSize))
-				elog(ERROR, "failed to add index item to \"%s\"", RelationGetRelationName(index));
-
-			/* Overwrite neighbor tuple */
-			if (!PageIndexTupleOverwrite(npage, neighborOffno, (Item) ntup, ntupSize))
-				elog(ERROR, "failed to add index item to \"%s\"", RelationGetRelationName(index));
+			/* We modified the tuples in place, no need to call PageIndexTupleOverwrite */
 
 			/* Commit */
 			GenericXLogFinish(state);