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/
>From 5eff6872a446c7a3e86607b5d73a18e1a95b7da7 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.von...@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->ii_Context</literal> and store a pointer to the
data in <literal>indexInfo->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->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
