Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 41d2c6f

Browse files
committed
Add missing index_insert_cleanup calls
The optimization for inserts into BRIN indexes added by c1ec02b relies on a cache that needs to be explicitly released after calling index_insert(). The commit however failed to invoke the cleanup in validate_index(), which calls index_insert() indirectly through table_index_validate_scan(). After inspecting index_insert() callers, it seems unique_key_recheck() is missing the call too. Fixed by adding the two missing index_insert_cleanup() calls. The commit does two additional improvements. The aminsertcleanup() signature is modified to have the index as the first argument, to make it more like the other AM callbacks. And the aminsertcleanup() callback is invoked even if the ii_AmCache is NULL, so that it can decide if the cleanup is necessary. Author: Alvaro Herrera, Tomas Vondra Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/202401091043.e3nrqiad6gb7@alvherre.pgsql
1 parent 95d14b7 commit 41d2c6f

File tree

9 files changed

+26
-16
lines changed

9 files changed

+26
-16
lines changed

doc/src/sgml/indexam.sgml

+7-7
Original file line numberDiff line numberDiff line change
@@ -367,21 +367,21 @@ aminsert (Relation indexRelation,
367367
within an SQL statement, it can allocate space
368368
in <literal>indexInfo-&gt;ii_Context</literal> and store a pointer to the
369369
data in <literal>indexInfo-&gt;ii_AmCache</literal> (which will be NULL
370-
initially). After the index insertions complete, the memory will be freed
371-
automatically. If additional cleanup is required (e.g. if the cache contains
372-
buffers and tuple descriptors), the AM may define an optional function
373-
<literal>aminsertcleanup</literal>, called before the memory is released.
370+
initially). If resources other than memory have to be released after
371+
index insertions, <function>aminsertcleanup</function> may be provided,
372+
which will be called before the memory is released.
374373
</para>
375374

376375
<para>
377376
<programlisting>
378377
void
379-
aminsertcleanup (IndexInfo *indexInfo);
378+
aminsertcleanup (Relation indexRelation,
379+
IndexInfo *indexInfo);
380380
</programlisting>
381381
Clean up state that was maintained across successive inserts in
382382
<literal>indexInfo-&gt;ii_AmCache</literal>. This is useful if the data
383-
requires additional cleanup steps, and simply releasing the memory is not
384-
sufficient.
383+
requires additional cleanup steps (e.g., releasing pinned buffers), and
384+
simply releasing the memory is not sufficient.
385385
</para>
386386

387387
<para>

src/backend/access/brin/brin.c

+4-2
Original file line numberDiff line numberDiff line change
@@ -500,11 +500,13 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
500500
* Callback to clean up the BrinInsertState once all tuple inserts are done.
501501
*/
502502
void
503-
brininsertcleanup(IndexInfo *indexInfo)
503+
brininsertcleanup(Relation index, IndexInfo *indexInfo)
504504
{
505505
BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
506506

507-
Assert(bistate);
507+
/* bail out if cache not initialized */
508+
if (indexInfo->ii_AmCache == NULL)
509+
return;
508510

509511
/*
510512
* Clean up the revmap. Note that the brinDesc has already been cleaned up

src/backend/access/index/indexam.c

+2-3
Original file line numberDiff line numberDiff line change
@@ -242,10 +242,9 @@ index_insert_cleanup(Relation indexRelation,
242242
IndexInfo *indexInfo)
243243
{
244244
RELATION_CHECKS;
245-
Assert(indexInfo);
246245

247-
if (indexRelation->rd_indam->aminsertcleanup && indexInfo->ii_AmCache)
248-
indexRelation->rd_indam->aminsertcleanup(indexInfo);
246+
if (indexRelation->rd_indam->aminsertcleanup)
247+
indexRelation->rd_indam->aminsertcleanup(indexRelation, indexInfo);
249248
}
250249

251250
/*

src/backend/catalog/index.c

+3
Original file line numberDiff line numberDiff line change
@@ -3402,6 +3402,9 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
34023402
/* Done with tuplesort object */
34033403
tuplesort_end(state.tuplesort);
34043404

3405+
/* Make sure to release resources cached in indexInfo (if needed). */
3406+
index_insert_cleanup(indexRelation, indexInfo);
3407+
34053408
elog(DEBUG2,
34063409
"validate_index found %.0f heap tuples, %.0f index tuples; inserted %.0f missing tuples",
34073410
state.htups, state.itups, state.tups_inserted);

src/backend/commands/constraint.c

+3
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,9 @@ unique_key_recheck(PG_FUNCTION_ARGS)
174174
index_insert(indexRel, values, isnull, &checktid,
175175
trigdata->tg_relation, UNIQUE_CHECK_EXISTING,
176176
false, indexInfo);
177+
178+
/* Cleanup cache possibly initialized by index_insert. */
179+
index_insert_cleanup(indexRel, indexInfo);
177180
}
178181
else
179182
{

src/include/access/amapi.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,8 @@ typedef bool (*aminsert_function) (Relation indexRelation,
114114
struct IndexInfo *indexInfo);
115115

116116
/* cleanup after insert */
117-
typedef void (*aminsertcleanup_function) (struct IndexInfo *indexInfo);
117+
typedef void (*aminsertcleanup_function) (Relation indexRelation,
118+
struct IndexInfo *indexInfo);
118119

119120
/* bulk delete */
120121
typedef IndexBulkDeleteResult *(*ambulkdelete_function) (IndexVacuumInfo *info,

src/include/access/brin_internal.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ extern bool brininsert(Relation idxRel, Datum *values, bool *nulls,
9696
IndexUniqueCheck checkUnique,
9797
bool indexUnchanged,
9898
struct IndexInfo *indexInfo);
99-
extern void brininsertcleanup(struct IndexInfo *indexInfo);
99+
extern void brininsertcleanup(Relation index, struct IndexInfo *indexInfo);
100100
extern IndexScanDesc brinbeginscan(Relation r, int nkeys, int norderbys);
101101
extern int64 bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
102102
extern void brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,

src/test/regress/expected/brin.out

+2-1
Original file line numberDiff line numberDiff line change
@@ -575,6 +575,7 @@ DROP TABLE brintest_unlogged;
575575
-- test that the insert optimization works if no rows end up inserted
576576
CREATE TABLE brin_insert_optimization (a int);
577577
INSERT INTO brin_insert_optimization VALUES (1);
578-
CREATE INDEX ON brin_insert_optimization USING brin (a);
578+
CREATE INDEX brin_insert_optimization_idx ON brin_insert_optimization USING brin (a);
579579
UPDATE brin_insert_optimization SET a = a;
580+
REINDEX INDEX CONCURRENTLY brin_insert_optimization_idx;
580581
DROP TABLE brin_insert_optimization;

src/test/regress/sql/brin.sql

+2-1
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,7 @@ DROP TABLE brintest_unlogged;
519519
-- test that the insert optimization works if no rows end up inserted
520520
CREATE TABLE brin_insert_optimization (a int);
521521
INSERT INTO brin_insert_optimization VALUES (1);
522-
CREATE INDEX ON brin_insert_optimization USING brin (a);
522+
CREATE INDEX brin_insert_optimization_idx ON brin_insert_optimization USING brin (a);
523523
UPDATE brin_insert_optimization SET a = a;
524+
REINDEX INDEX CONCURRENTLY brin_insert_optimization_idx;
524525
DROP TABLE brin_insert_optimization;

0 commit comments

Comments
 (0)