On 12/11/23 16:41, Tomas Vondra wrote:
> On 12/11/23 16:00, Alexander Lakhin wrote:
>> Hello Tomas and Soumyadeep,
>>
>> 25.11.2023 23:06, Tomas Vondra wrote:
>>> I've done a bit more cleanup on the last version of the patch (renamed
>>> the fields to start with bis_ as agreed, rephrased the comments / docs /
>>> commit message a bit) and pushed.
>>
>> Please look at a query, which produces warnings similar to the ones
>> observed upthread:
>> CREATE TABLE t(a INT);
>> INSERT INTO t SELECT x FROM generate_series(1,10000) x;
>> CREATE INDEX idx ON t USING brin (a);
>> REINDEX index CONCURRENTLY idx;
>>
>> WARNING: resource was not closed: [1863] (rel=base/16384/16389,
>> blockNum=1, flags=0x93800000, refcount=1 1)
>> WARNING: resource was not closed: [1862] (rel=base/16384/16389,
>> blockNum=0, flags=0x93800000, refcount=1 1)
>>
>> The first bad commit for this anomaly is c1ec02be1.
>>
>
> Thanks for the report. I haven't investigated what the issue is, but it
> seems we fail to release the buffers in some situations - I'd bet we
> fail to call the cleanup callback in some place, or something like that.
> I'll take a look tomorrow.
>
Yeah, just as I expected this seems to be a case of failing to call the
index_insert_cleanup after doing some inserts - in this case the inserts
happen in table_index_validate_scan, but validate_index has no idea it
needs to do the cleanup.
The attached 0001 patch fixes this by adding the call to validate_index,
which seems like the proper place as it's where the indexInfo is
allocated and destroyed.
But this makes me think - are there any other places that might call
index_insert without then also doing the cleanup? I did grep for the
index_insert() calls, and it seems OK except for this one.
There's a call in toast_internals.c, but that seems OK because that only
deals with btree indexes (and those don't need any cleanup). The same
logic applies to unique_key_recheck(). The rest goes through
execIndexing.c, which should do the cleanup in ExecCloseIndices().
Note: We should probably call the cleanup even in the btree cases, even
if only to make it clear it needs to be called after index_insert().
I was thinking maybe we should have some explicit call to destroy the
IndexInfo, but that seems rather inconvenient - it'd force everyone to
carefully track lifetimes of the IndexInfo instead of just relying on
memory context reset/destruction. That seems quite error-prone.
I propose we do a much simpler thing instead - allow the cache may be
initialized / cleaned up repeatedly, and make sure it gets reset at
convenient place (typically after index_insert calls that don't go
through execIndexing). That'd mean the cleanup does not need to happen
very far from the index_insert(), which makes the reasoning much easier.
0002 does this.
But maybe there's a better way to ensure the cleanup gets called even
when not using execIndexing.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From 0cf18dd53842e3809b76525867214ce8ff823f32 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Tue, 12 Dec 2023 12:01:07 +0100
Subject: [PATCH 1/2] call cleanup in validate_index
---
src/backend/catalog/index.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7b186c0220b..7a0e337a418 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);
--
2.42.0
From 0c04b53573cf164c5a0108d909f05ffcbae4e72d Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Tue, 12 Dec 2023 12:24:05 +0100
Subject: [PATCH 2/2] call index_insert_cleanup repeatedly
---
src/backend/access/heap/heapam_handler.c | 3 +++
src/backend/access/index/indexam.c | 11 ++++++++++-
src/backend/catalog/index.c | 2 --
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7c28dafb728..5db0cf1dffa 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -1976,6 +1976,9 @@ heapam_index_validate_scan(Relation heapRelation,
/* These may have been pointing to the now-gone estate */
indexInfo->ii_ExpressionsState = NIL;
indexInfo->ii_PredicateState = NULL;
+
+ /* also make sure the IndexInfo cache gets freed */
+ index_insert_cleanup(indexRelation, indexInfo);
}
/*
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index f23e0199f08..515d7649c4d 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -207,8 +207,17 @@ index_insert_cleanup(Relation indexRelation,
RELATION_CHECKS;
Assert(indexInfo);
- if (indexRelation->rd_indam->aminsertcleanup && indexInfo->ii_AmCache)
+ if (!indexInfo->ii_AmCache)
+ return;
+
+ if (indexRelation->rd_indam->aminsertcleanup)
indexRelation->rd_indam->aminsertcleanup(indexInfo);
+
+ if (indexInfo->ii_AmCache)
+ {
+ pfree(indexInfo->ii_AmCache);
+ indexInfo->ii_AmCache = NULL;
+ }
}
/*
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7a0e337a418..7b186c0220b 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3414,8 +3414,6 @@ 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);
--
2.42.0
