Feedback-ID: ia2694551:Fastmail
Date: Tue, 9 Jan 2024 11:43:56 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Alexander Lakhin <exclusion@gmail.com>,
	Soumyadeep Chakraborty <soumyadeep2007@gmail.com>,
 Matthias van de Meent <boekewurm+postgres@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
 Ashwin Agrawal <ashwinstar@gmail.com>
Subject: Re: brininsert optimization opportunity
Message-ID: <202401091043.e3nrqiad6gb7@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="lvz367sfohxlwjc7"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202401081551.3olwx6pyw2mn@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202401091043.e3nrqiad6gb7%40alvherre.pgsql>
Precedence: bulk


--lvz367sfohxlwjc7
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2024-Jan-08, Alvaro Herrera wrote:

> So I think we should do 0001 and perhaps some further tweaks to the
> original brininsert optimization commit: [...]

So I propose the attached patch, which should fix the reported bug and
the things I mentioned above, and also the typos Alexander mentioned
elsewhere in the thread.

> Lastly, I kinda disagree with the notion that only some of the callers
> of aminsert should call aminsertcleanup, even though btree doesn't have
> an aminsertcleanup and thus it can't affect TOAST or catalogs. [...]

I didn't do anything about this.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

--lvz367sfohxlwjc7
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0001-call-cleanup-in-validate_index.patch"

From 5eff6872a446c7a3e86607b5d73a18e1a95b7da7 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Tue, 12 Dec 2023 12:01:07 +0100
Subject: [PATCH v2] call cleanup in validate_index

---
 doc/src/sgml/indexam.sgml          | 14 +++++++-------
 src/backend/access/brin/brin.c     | 19 +++++++++++--------
 src/backend/access/index/indexam.c |  5 ++---
 src/backend/catalog/index.c        |  2 ++
 src/include/access/amapi.h         |  2 +-
 src/include/access/brin_internal.h |  2 +-
 6 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index cc4135e394..4817acd31f 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -367,21 +367,21 @@ aminsert (Relation indexRelation,
    within an SQL statement, it can allocate space
    in <literal>indexInfo-&gt;ii_Context</literal> and store a pointer to the
    data in <literal>indexInfo-&gt;ii_AmCache</literal> (which will be NULL
-   initially). After the index insertions complete, the memory will be freed
-   automatically. If additional cleanup is required (e.g. if the cache contains
-   buffers and tuple descriptors), the AM may define an optional function
-   <literal>indexinsertcleanup</literal>, called before the memory is released.
+   initially).  If resources other than memory have to be released
+   after index insertions, <function>aminsertcleanup</function> may
+   be provided, which will be called before the memory is released.
   </para>
 
   <para>
 <programlisting>
 void
-aminsertcleanup (IndexInfo *indexInfo);
+aminsertcleanup (Relation indexRelation,
+                 IndexInfo *indexInfo);
 </programlisting>
    Clean up state that was maintained across successive inserts in
    <literal>indexInfo-&gt;ii_AmCache</literal>. This is useful if the data
-   requires additional cleanup steps, and simply releasing the memory is not
-   sufficient.
+   requires additional cleanup steps (e.g., releasing pinned
+   buffers), and simply releasing the memory is not sufficient.
   </para>
 
   <para>
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 1087a9011e..6ac12b9ba8 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -222,7 +222,7 @@ static bool add_values_to_range(Relation idxRel, BrinDesc *bdesc,
 								BrinMemTuple *dtup, const Datum *values, const bool *nulls);
 static bool check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys);
 static void brin_fill_empty_ranges(BrinBuildState *state,
-								   BlockNumber prevRange, BlockNumber maxRange);
+								   BlockNumber prevRange, BlockNumber nextRange);
 
 /* parallel index builds */
 static void _brin_begin_parallel(BrinBuildState *buildstate, Relation heap, Relation index,
@@ -498,11 +498,14 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
  * Callback to clean up the BrinInsertState once all tuple inserts are done.
  */
 void
-brininsertcleanup(IndexInfo *indexInfo)
+brininsertcleanup(Relation idx, IndexInfo *indexInfo)
 {
-	BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
+	BrinInsertState *bistate;
 
-	Assert(bistate);
+	if (indexInfo->ii_AmCache == NULL)
+		return;
+
+	bistate = (BrinInsertState *) indexInfo->ii_AmCache;
 
 	/*
 	 * Clean up the revmap. Note that the brinDesc has already been cleaned up
@@ -1149,8 +1152,8 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 	 *
 	 * XXX plan_create_index_workers makes the number of workers dependent on
 	 * maintenance_work_mem, requiring 32MB for each worker. That makes sense
-	 * for btree, but not for BRIN, which can do away with much less memory.
-	 * So maybe make that somehow less strict, optionally?
+	 * for btree, but not for BRIN, which can do with much less memory.  So
+	 * maybe make that somehow less strict, optionally?
 	 */
 	if (indexInfo->ii_ParallelWorkers > 0)
 		_brin_begin_parallel(state, heap, index, indexInfo->ii_Concurrent,
@@ -2543,7 +2546,7 @@ _brin_end_parallel(BrinLeader *brinleader, BrinBuildState *state)
 
 	/*
 	 * Initialize BrinMemTuple we'll use to union summaries from workers (in
-	 * case they happened to produce parts of the same paga range).
+	 * case they happened to produce parts of the same page range).
 	 */
 	memtuple = brin_new_memtuple(state->bs_bdesc);
 
@@ -2867,7 +2870,7 @@ _brin_parallel_build_main(dsm_segment *seg, shm_toc *toc)
  * specified block number. The empty tuple is initialized only once, when it's
  * needed for the first time, stored in the memory context bs_context to ensure
  * proper life span, and reused on following calls. All empty tuples are
- * exactly the same except for the bs_blkno field, which is set to the value
+ * exactly the same except for the bt_blkno field, which is set to the value
  * in blkno parameter.
  */
 static void
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index 63dff101e2..ff84bf0fa0 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -205,10 +205,9 @@ index_insert_cleanup(Relation indexRelation,
 					 IndexInfo *indexInfo)
 {
 	RELATION_CHECKS;
-	Assert(indexInfo);
 
-	if (indexRelation->rd_indam->aminsertcleanup && indexInfo->ii_AmCache)
-		indexRelation->rd_indam->aminsertcleanup(indexInfo);
+	if (indexRelation->rd_indam->aminsertcleanup)
+		indexRelation->rd_indam->aminsertcleanup(indexRelation, indexInfo);
 }
 
 /*
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 88f7994b5a..40ac683b35 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3414,6 +3414,8 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
 	/* Done with tuplesort object */
 	tuplesort_end(state.tuplesort);
 
+	index_insert_cleanup(indexRelation, indexInfo);
+
 	elog(DEBUG2,
 		 "validate_index found %.0f heap tuples, %.0f index tuples; inserted %.0f missing tuples",
 		 state.htups, state.itups, state.tups_inserted);
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 2c6c307efc..683ad13f07 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -114,7 +114,7 @@ typedef bool (*aminsert_function) (Relation indexRelation,
 								   struct IndexInfo *indexInfo);
 
 /* cleanup after insert */
-typedef void (*aminsertcleanup_function) (struct IndexInfo *indexInfo);
+typedef void (*aminsertcleanup_function) (Relation indexRelation, struct IndexInfo *indexInfo);
 
 /* bulk delete */
 typedef IndexBulkDeleteResult *(*ambulkdelete_function) (IndexVacuumInfo *info,
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 1c7eabe604..a5a9772621 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -96,7 +96,7 @@ extern bool brininsert(Relation idxRel, Datum *values, bool *nulls,
 					   IndexUniqueCheck checkUnique,
 					   bool indexUnchanged,
 					   struct IndexInfo *indexInfo);
-extern void brininsertcleanup(struct IndexInfo *indexInfo);
+extern void brininsertcleanup(Relation index, struct IndexInfo *indexInfo);
 extern IndexScanDesc brinbeginscan(Relation r, int nkeys, int norderbys);
 extern int64 bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
 extern void brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
-- 
2.39.2


--lvz367sfohxlwjc7--