On 2023-Dec-12, Tomas Vondra wrote: > 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.
I'm not in love with this 0002 patch; I think the layering after 0001 is correct in that the insert_cleanup call should remain in validate_index and called after the whole thing is done, but 0002 changes things so that now every table AM has to worry about doing this correctly; and a bug of omission will not be detected unless you have a BRIN index on such a table and happen to use CREATE INDEX CONCURRENTLY. So a developer has essentially zero chance to do things correctly, which I think we'd rather avoid. So I think we should do 0001 and perhaps some further tweaks to the original brininsert optimization commit: I think the aminsertcleanup callback should receive the indexRelation as first argument; and also I think it's not index_insert_cleanup() job to worry about whether ii_AmCache is NULL or not, but instead the callback should be invoked always, and then it's aminsertcleanup job to do nothing if ii_AmCache is NULL. That way, the index AM API doesn't have to worry about which parts of IndexInfo (or the indexRelation) is aminsertcleanup going to care about. If we decide to change this, then the docs also need a bit of tweaking I think. 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. Maybe we can turn index_insert_cleanup into an inline function, which can quickly do nothing if aminsertcleanup isn't defined. Then we no longer have the layering violation where we assume that btree doesn't care. But the proposed change in this paragraph can be maybe handled separately to avoid confusing things with the bugfix in the two paragraphs above. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ Essentially, you're proposing Kevlar shoes as a solution for the problem that you want to walk around carrying a loaded gun aimed at your foot. (Tom Lane)