Hi,
I took a look at this patch today. I had to rebase the patch, due to
some minor bitrot related to 9f0602539d (but nothing major). I also did
a couple tiny cosmetic tweaks, but other than that the patch seems OK.
See the attached v6.
I did some simple performance tests too, similar to those in the initial
message:
CREATE UNLOGGED TABLE heap (i int);
CREATE INDEX ON heap USING brin(i) WITH (pages_per_range=1);
--
TRUNCATE heap;
INSERT INTO heap SELECT 1 FROM generate_series(1, 20000000);
And the results look like this (5 runs each):
master: 16448.338 16066.473 16039.166 16067.420 16080.066
patched: 13260.065 13229.800 13254.454 13265.479 13273.693
So that's a nice improvement, even though enabling WAL will make the
relative speedup somewhat smaller.
The one thing I'm not entirely sure about is adding new stuff to the
IndexAmRoutine. I don't think we want to end up with too many callbacks
that all AMs have to initialize etc. I can't think of a different/better
way to do this, though.
Barring objections, I'll try to push this early next week, after another
round of cleanup.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From b1c47527132f0e8c39ff221b6b5adcc9678ba6ce Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.von...@postgresql.org>
Date: Fri, 3 Nov 2023 19:17:21 +0100
Subject: [PATCH v6] Reuse revmap and brin desc in brininsert
brininsert() used to have code that performed per-tuple initialization
of the revmap. That had some overhead.
To mitigate, we introduce a new AM callback to clean up state at the end
of all inserts, and perform the revmap cleanup there.
---
contrib/bloom/blutils.c | 1 +
doc/src/sgml/indexam.sgml | 14 +++++-
src/backend/access/brin/brin.c | 74 +++++++++++++++++++++++-----
src/backend/access/gin/ginutil.c | 1 +
src/backend/access/gist/gist.c | 1 +
src/backend/access/hash/hash.c | 1 +
src/backend/access/index/indexam.c | 15 ++++++
src/backend/access/nbtree/nbtree.c | 1 +
src/backend/access/spgist/spgutils.c | 1 +
src/backend/executor/execIndexing.c | 5 ++
src/include/access/amapi.h | 4 ++
src/include/access/brin_internal.h | 1 +
src/include/access/genam.h | 2 +
13 files changed, 109 insertions(+), 12 deletions(-)
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index f23fbb1d9e0..4830cb3fee6 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -131,6 +131,7 @@ blhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = blbuild;
amroutine->ambuildempty = blbuildempty;
amroutine->aminsert = blinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = blbulkdelete;
amroutine->amvacuumcleanup = blvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 30eda37afa8..a36d2eaebe7 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -139,6 +139,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
@@ -359,11 +360,22 @@ 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).
+ initially). If the data cached contains structures that can be simply pfree'd,
+ they will implicitly be pfree'd. But, if it contains more complex data, such
+ as Buffers or TupleDescs, additional cleanup is necessary. This additional
+ cleanup can be performed in <literal>indexinsertcleanup</literal>.
</para>
<para>
<programlisting>
+bool
+aminsertcleanup (IndexInfo *indexInfo);
+</programlisting>
+ Clean up state that was maintained across successive inserts in
+ <literal>indexInfo->ii_AmCache</literal>. This is useful if the data
+ contained is complex - like Buffers or TupleDescs which need additional
+ cleanup, unlike simple structures that will be implicitly pfree'd.
+<programlisting>
IndexBulkDeleteResult *
ambulkdelete (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 25338a90e29..fd99b6eb11e 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -58,6 +58,17 @@ typedef struct BrinBuildState
BrinMemTuple *bs_dtuple;
} BrinBuildState;
+/*
+ * We use a BrinInsertState to capture running state spanning multiple
+ * brininsert invocations, within the same command.
+ */
+typedef struct BrinInsertState
+{
+ BrinRevmap *bs_rmAccess;
+ BrinDesc *bs_desc;
+ BlockNumber bs_pages_per_range;
+} BrinInsertState;
+
/*
* Struct used as "opaque" during index scans
*/
@@ -72,6 +83,7 @@ typedef struct BrinOpaque
static BrinBuildState *initialize_brin_buildstate(Relation idxRel,
BrinRevmap *revmap, BlockNumber pagesPerRange);
+static BrinInsertState *initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo);
static void terminate_brin_buildstate(BrinBuildState *state);
static void brinsummarize(Relation index, Relation heapRel, BlockNumber pageRange,
bool include_partial, double *numSummarized, double *numExisting);
@@ -117,6 +129,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = brinbuild;
amroutine->ambuildempty = brinbuildempty;
amroutine->aminsert = brininsert;
+ amroutine->aminsertcleanup = brininsertcleanup;
amroutine->ambulkdelete = brinbulkdelete;
amroutine->amvacuumcleanup = brinvacuumcleanup;
amroutine->amcanreturn = NULL;
@@ -140,6 +153,27 @@ brinhandler(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(amroutine);
}
+/*
+ * Initialize a BrinInsertState to maintain state to be used across multiple
+ * tuple inserts, within the same command.
+ */
+static BrinInsertState *
+initialize_brin_insertstate(Relation idxRel, IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate;
+ MemoryContext oldcxt;
+
+ oldcxt = MemoryContextSwitchTo(indexInfo->ii_Context);
+ bistate = palloc0(sizeof(BrinInsertState));
+ bistate->bs_desc = brin_build_desc(idxRel);
+ bistate->bs_rmAccess = brinRevmapInitialize(idxRel,
+ &bistate->bs_pages_per_range);
+ indexInfo->ii_AmCache = bistate;
+ MemoryContextSwitchTo(oldcxt);
+
+ return bistate;
+}
+
/*
* A tuple in the heap is being inserted. To keep a brin index up to date,
* we need to obtain the relevant index tuple and compare its stored values
@@ -162,14 +196,23 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
BlockNumber pagesPerRange;
BlockNumber origHeapBlk;
BlockNumber heapBlk;
- BrinDesc *bdesc = (BrinDesc *) indexInfo->ii_AmCache;
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
BrinRevmap *revmap;
+ BrinDesc *bdesc;
Buffer buf = InvalidBuffer;
MemoryContext tupcxt = NULL;
MemoryContext oldcxt = CurrentMemoryContext;
bool autosummarize = BrinGetAutoSummarize(idxRel);
- revmap = brinRevmapInitialize(idxRel, &pagesPerRange);
+ if (!bistate)
+ {
+ /* First time through in this statement? */
+ bistate = initialize_brin_insertstate(idxRel, indexInfo);
+ }
+
+ revmap = bistate->bs_rmAccess;
+ bdesc = bistate->bs_desc;
+ pagesPerRange = bistate->bs_pages_per_range;
/*
* origHeapBlk is the block number where the insertion occurred. heapBlk
@@ -228,14 +271,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
if (!brtup)
break;
- /* First time through in this statement? */
- if (bdesc == NULL)
- {
- MemoryContextSwitchTo(indexInfo->ii_Context);
- bdesc = brin_build_desc(idxRel);
- indexInfo->ii_AmCache = (void *) bdesc;
- MemoryContextSwitchTo(oldcxt);
- }
/* First time through in this brininsert call? */
if (tupcxt == NULL)
{
@@ -306,7 +341,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
break;
}
- brinRevmapTerminate(revmap);
if (BufferIsValid(buf))
ReleaseBuffer(buf);
MemoryContextSwitchTo(oldcxt);
@@ -316,6 +350,24 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
return false;
}
+/*
+ * Callback to clean up the BrinInsertState once all tuple inserts are done.
+ */
+void
+brininsertcleanup(IndexInfo *indexInfo)
+{
+ BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
+
+ Assert(bistate);
+ /*
+ * Clean up the revmap. Note that the brinDesc has already been cleaned up
+ * as part of its own memory context.
+ */
+ brinRevmapTerminate(bistate->bs_rmAccess);
+ bistate->bs_rmAccess = NULL;
+ bistate->bs_desc = NULL;
+}
+
/*
* Initialize state for a BRIN index scan.
*
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 7a4cd93f301..a875c5d3d7a 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -64,6 +64,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = ginbuild;
amroutine->ambuildempty = ginbuildempty;
amroutine->aminsert = gininsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = ginbulkdelete;
amroutine->amvacuumcleanup = ginvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 8ef5fa03290..9a1bf8f66cb 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -86,6 +86,7 @@ gisthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = gistbuild;
amroutine->ambuildempty = gistbuildempty;
amroutine->aminsert = gistinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = gistbulkdelete;
amroutine->amvacuumcleanup = gistvacuumcleanup;
amroutine->amcanreturn = gistcanreturn;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 7a025f33cfe..6443ff21bda 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -83,6 +83,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->ambuild = hashbuild;
amroutine->ambuildempty = hashbuildempty;
amroutine->aminsert = hashinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = hashbulkdelete;
amroutine->amvacuumcleanup = hashvacuumcleanup;
amroutine->amcanreturn = NULL;
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index b25b03f7abc..597b763d1f6 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -196,6 +196,21 @@ index_insert(Relation indexRelation,
indexInfo);
}
+/* -------------------------
+ * index_insert_cleanup - clean up after all index inserts are done
+ * -------------------------
+ */
+void
+index_insert_cleanup(Relation indexRelation,
+ IndexInfo *indexInfo)
+{
+ RELATION_CHECKS;
+ Assert(indexInfo);
+
+ if (indexRelation->rd_indam->aminsertcleanup)
+ indexRelation->rd_indam->aminsertcleanup(indexInfo);
+}
+
/*
* index_beginscan - start a scan of an index with amgettuple
*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index a88b36a589a..0930f9b37e3 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -122,6 +122,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->ambuild = btbuild;
amroutine->ambuildempty = btbuildempty;
amroutine->aminsert = btinsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = btbulkdelete;
amroutine->amvacuumcleanup = btvacuumcleanup;
amroutine->amcanreturn = btcanreturn;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index c112e1e5dd4..30c00876a56 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -70,6 +70,7 @@ spghandler(PG_FUNCTION_ARGS)
amroutine->ambuild = spgbuild;
amroutine->ambuildempty = spgbuildempty;
amroutine->aminsert = spginsert;
+ amroutine->aminsertcleanup = NULL;
amroutine->ambulkdelete = spgbulkdelete;
amroutine->amvacuumcleanup = spgvacuumcleanup;
amroutine->amcanreturn = spgcanreturn;
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 384b39839a0..2fa2118f3c2 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -233,15 +233,20 @@ ExecCloseIndices(ResultRelInfo *resultRelInfo)
int i;
int numIndices;
RelationPtr indexDescs;
+ IndexInfo **indexInfos;
numIndices = resultRelInfo->ri_NumIndices;
indexDescs = resultRelInfo->ri_IndexRelationDescs;
+ indexInfos = resultRelInfo->ri_IndexRelationInfo;
for (i = 0; i < numIndices; i++)
{
if (indexDescs[i] == NULL)
continue; /* shouldn't happen? */
+ /* Give the index a chance to do some post-insert cleanup */
+ index_insert_cleanup(indexDescs[i], indexInfos[i]);
+
/* Drop lock acquired by ExecOpenIndices */
index_close(indexDescs[i], RowExclusiveLock);
}
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 995725502a6..244459587fc 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -113,6 +113,9 @@ typedef bool (*aminsert_function) (Relation indexRelation,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+/* cleanup after insert */
+typedef void (*aminsertcleanup_function) (struct IndexInfo *indexInfo);
+
/* bulk delete */
typedef IndexBulkDeleteResult *(*ambulkdelete_function) (IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
@@ -261,6 +264,7 @@ typedef struct IndexAmRoutine
ambuild_function ambuild;
ambuildempty_function ambuildempty;
aminsert_function aminsert;
+ aminsertcleanup_function aminsertcleanup;
ambulkdelete_function ambulkdelete;
amvacuumcleanup_function amvacuumcleanup;
amcanreturn_function amcanreturn; /* can be NULL */
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 97ddc925b27..ed231e208eb 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -96,6 +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 IndexScanDesc brinbeginscan(Relation r, int nkeys, int norderbys);
extern int64 bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern void brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index f31dec6ee0f..80dc8d54066 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -148,6 +148,8 @@ extern bool index_insert(Relation indexRelation,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
+extern void index_insert_cleanup(Relation indexRelation,
+ struct IndexInfo *indexInfo);
extern IndexScanDesc index_beginscan(Relation heapRelation,
Relation indexRelation,
--
2.41.0
