Hi,
Here's two patched to deal with this open item. 0001 is a trivial fix of
typos and wording, I moved it into a separate commit for clarity. 0002
does the actual fix - adding the index_insert_cleanup(). It's 99% the
patch Alvaro shared some time ago, with only some minor formatting
tweaks by me.
I've also returned to this Alvaro's comment:
> 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.
which was a reaction to my earlier statement about places calling
index_insert():
> 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().
I think Alvaro is right, so I went through all index_insert() callers
and checked which need the cleanup. Luckily there's not that many of
them, only 5 in total call index_insert() directly:
1) toast_save_datum (src/backend/access/common/toast_internals.c)
This is safe, because the index_insert() passes indexInfo=NULL, so
there can't possibly be any cache. If we ever decide to pass a valid
indexInfo, we can add the cleanup, now it seems pointless.
Note: If someone created a BRIN index on a TOAST table, that'd already
crash, because BRIN blindly dereferences the indexInfo. Maybe that
should be fixed, but we don't support CREATE INDEX on TOAST tables.
2) heapam_index_validate_scan (src/backend/access/heap/heapam_handler.c)
Covered by the committed fix, adding cleanup to validate_index.
3) CatalogIndexInsert (src/backend/catalog/indexing.c)
Covered by all callers also calling CatalogCloseIndexes, which in turn
calls ExecCloseIndices and cleanup.
4) unique_key_recheck (src/backend/commands/constraint.c)
This seems like the only place missing the cleanup call.
5) ExecInsertIndexTuples (src/backend/executor/execIndexing.c)
Should be covered by ExecCloseIndices, called after the insertions.
So it seems only (4) unique_key_recheck needs the extra call (it can't
really happen higher because the indexInfo is a local variable). So the
0002 patch adds the call.
The patch also adds a test for this (or rather tweaks an existing one).
It's a bit too late for me to push this now, I'll do so early tomorrow.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company