v2-0001-Fix-for-GiST-buffered-build-bug-related-to-contex.patch

application/octet-stream

Filename: v2-0001-Fix-for-GiST-buffered-build-bug-related-to-contex.patch
Type: application/octet-stream
Part: 0
Message: Re: BUG #16329: Valgrind detects an invalid read when building a gist index with buffering

Patch

Format: format-patch
Series: patch v2-0001
Subject: Fix for GiST buffered build bug related to context resets inside a calls tree which can render itup pointer invalid
File+
src/backend/access/gist/gistbuild.c 15 6
From 6a12161c17052601ec7e7a398ec691adec7f48fb Mon Sep 17 00:00:00 2001
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Mon, 2 Nov 2020 20:07:06 +0400
Subject: [PATCH v2] Fix for GiST buffered build bug related to context resets
 inside a calls tree which can render itup pointer invalid

---
 src/backend/access/gist/gistbuild.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 9d3fa9c3b7..94d65b39d8 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -818,10 +818,24 @@ gistBuildCallback(Relation index,
 						 true);
 	itup->t_tid = *tid;
 
+	/* Update tuple count and total size. */
+	buildstate->indtuples += 1;
+	buildstate->indtuplesSize += IndexTupleSize(itup);
+
+	/*
+	 * XXX Generally, memory context better be 'owned' by only one function.
+	 * If the context is reset somewhere inside its calls, pointers can
+	 * become dangling just immediately. Though in the case of
+	 * gistBufferingBuildInsert, tempCxt can be reset inside
+	 * gistProcessEmptyingQueue because it is the last operation inside the
+	 * call and no pointers are used after it, so we can allow this piece of
+	 * magic.
+	 */
 	if (buildstate->buildMode == GIST_BUFFERING_ACTIVE)
 	{
 		/* We have buffers, so use them. */
 		gistBufferingBuildInsert(buildstate, itup);
+		/* tempCxt is already reset in gistProcessEmptyingQueue */
 	}
 	else
 	{
@@ -831,14 +845,9 @@ gistBuildCallback(Relation index,
 		 */
 		gistdoinsert(index, itup, buildstate->freespace,
 					 buildstate->giststate, buildstate->heaprel, true);
+		MemoryContextReset(buildstate->giststate->tempCxt);
 	}
-
-	/* Update tuple count and total size. */
-	buildstate->indtuples += 1;
-	buildstate->indtuplesSize += IndexTupleSize(itup);
-
 	MemoryContextSwitchTo(oldCtx);
-	MemoryContextReset(buildstate->giststate->tempCxt);
 
 	if (buildstate->buildMode == GIST_BUFFERING_ACTIVE &&
 		buildstate->indtuples % BUFFERING_MODE_TUPLE_SIZE_STATS_TARGET == 0)
-- 
2.28.0