v1-0001-Fix-memory-leak-in-indexUnchanged-hint-mechanism.patch

From bda9cf57a93ee6d6e1ad4147bae8f53e6f2e8e25 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Mon, 10 Jan 2022 11:28:10 -0800
Subject: [PATCH v1] Fix memory leak in indexUnchanged hint mechanism.

Oversight in commit 9dc718bd, which added the hinting mechanism.

Author: Peter Geoghegan <pg@bowt.ie>
Reported-By: Klaudie Willis <Klaudie.Willis@protonmail.com>
Diagnosed-By: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/261065.1639497535@sss.pgh.pa.us
Backpatch: 14-, where the hinting mechanism was added.
---
 src/include/nodes/execnodes.h       |  4 ++++
 src/backend/catalog/toasting.c      |  2 ++
 src/backend/executor/execIndexing.c | 25 +++++++++++++++++++++++--
 src/backend/nodes/makefuncs.c       |  2 ++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 4ff98f404..a2a2d12af 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -142,6 +142,8 @@ typedef struct ExprState
  *		Unique				is it a unique index?
  *		OpclassOptions		opclass-specific options, or NULL if none
  *		ReadyForInserts		is it valid for inserts?
+ *		CheckedUnchanged	IndexUnchanged status determined yet?
+ *		IndexUnchanged		aminsert hint, cached for retail inserts
  *		Concurrent			are we doing a concurrent index build?
  *		BrokenHotChain		did we detect any broken HOT chains?
  *		ParallelWorkers		# of workers requested (excludes leader)
@@ -172,6 +174,8 @@ typedef struct IndexInfo
 	Datum	   *ii_OpclassOptions;	/* array with one entry per column */
 	bool		ii_Unique;
 	bool		ii_ReadyForInserts;
+	bool		ii_CheckedUnchanged;
+	bool		ii_IndexUnchanged;
 	bool		ii_Concurrent;
 	bool		ii_BrokenHotChain;
 	int			ii_ParallelWorkers;
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 0db90c201..97ad33e0e 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -302,6 +302,8 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
 	indexInfo->ii_OpclassOptions = NULL;
 	indexInfo->ii_Unique = true;
 	indexInfo->ii_ReadyForInserts = true;
+	indexInfo->ii_CheckedUnchanged = false;
+	indexInfo->ii_IndexUnchanged = false;
 	indexInfo->ii_Concurrent = false;
 	indexInfo->ii_BrokenHotChain = false;
 	indexInfo->ii_ParallelWorkers = 0;
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index afe7ce87d..c5e8e751f 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -935,19 +935,32 @@ static bool
 index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
 						  IndexInfo *indexInfo, Relation indexRelation)
 {
-	Bitmapset  *updatedCols = ExecGetUpdatedCols(resultRelInfo, estate);
-	Bitmapset  *extraUpdatedCols = ExecGetExtraUpdatedCols(resultRelInfo, estate);
+	Bitmapset  *updatedCols;
+	Bitmapset  *extraUpdatedCols;
 	Bitmapset  *allUpdatedCols;
 	bool		hasexpression = false;
 	List	   *idxExprs;
 
+	/*
+	 * Check cache first
+	 */
+	if (indexInfo->ii_CheckedUnchanged)
+		return indexInfo->ii_IndexUnchanged;
+	indexInfo->ii_CheckedUnchanged = true;
+
 	/*
 	 * Check for indexed attribute overlap with updated columns.
 	 *
 	 * Only do this for key columns.  A change to a non-key column within an
 	 * INCLUDE index should not be counted here.  Non-key column values are
 	 * opaque payload state to the index AM, a little like an extra table TID.
+	 *
+	 * Note that row-level BEFORE triggers won't affect our behavior, since
+	 * they don't affect the updatedCols bitmaps generally.  It doesn't seem
+	 * worth the trouble of checking which attributes were changed directly.
 	 */
+	updatedCols = ExecGetUpdatedCols(resultRelInfo, estate);
+	extraUpdatedCols = ExecGetExtraUpdatedCols(resultRelInfo, estate);
 	for (int attr = 0; attr < indexInfo->ii_NumIndexKeyAttrs; attr++)
 	{
 		int			keycol = indexInfo->ii_IndexAttrNumbers[attr];
@@ -968,6 +981,7 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
 						  extraUpdatedCols))
 		{
 			/* Changed key column -- don't hint for this index */
+			indexInfo->ii_IndexUnchanged = false;
 			return false;
 		}
 	}
@@ -981,7 +995,10 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
 	 * shows that the index as a whole is logically unchanged by UPDATE.
 	 */
 	if (!hasexpression)
+	{
+		indexInfo->ii_IndexUnchanged = true;
 		return true;
+	}
 
 	/*
 	 * Need to pass only one bms to expression_tree_walker helper function.
@@ -1008,8 +1025,12 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
 		bms_free(allUpdatedCols);
 
 	if (hasexpression)
+	{
+		indexInfo->ii_IndexUnchanged = false;
 		return false;
+	}
 
+	indexInfo->ii_IndexUnchanged = true;
 	return true;
 }
 
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 7d1a01d1e..ac384843b 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -751,6 +751,8 @@ makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions,
 	Assert(n->ii_NumIndexKeyAttrs <= n->ii_NumIndexAttrs);
 	n->ii_Unique = unique;
 	n->ii_ReadyForInserts = isready;
+	n->ii_CheckedUnchanged = false;
+	n->ii_IndexUnchanged = false;
 	n->ii_Concurrent = concurrent;
 
 	/* expressions */
-- 
2.30.2