On 2024-Feb-13, Tomas Vondra wrote: > One thing that is not very clear to me is that I don't think there's a > very good way to determine which places need the cleanup call. Because > it depends on (a) what kind of index is used and (b) what happens in the > code called earlier (which may easily do arbitrary stuff). Which means > we have to call the cleanup whenever the code *might* have done inserts > into the index. Maybe it's not such an issue in practice, though.
I think it's not an issue, or rather that we should not try to guess. Instead make it a simple rule: if aminsert is called, then aminsertcleanup must be called afterwards, period. > On 1/8/24 16:51, Alvaro Herrera wrote: > > 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. [...] > I don't quite see why we should invoke the callback with ii_AmCache=NULL > though. If there's nothing cached, why bother? It just means all cleanup > callbacks have to do this NULL check on their own. Guessing that aminsertcleanup is not needed when ii_AmCache is NULL seems like a leaky abstraction. I propose to have only the AM know whether the cleanup call is important or not, without index_insert_cleanup assuming that it's related to ii_AmCache. Somebody could decide to have something completely different during insert cleanup, which is not in ii_AmCache. > I wonder if there's a nice way to check this in assert-enabled builds? > Could we tweak nbtree (or index AM in general) to check that all places > that called aminsert also called aminsertcleanup? > > For example, I was thinking we might add a flag to IndexInfo (separate > from the ii_AmCache), tracking if aminsert() was called, and Then later > check the aminsertcleanup() got called too. The problem however is > there's no obviously convenient place for this check, because IndexInfo > is not freed explicitly ... I agree it would be nice to have a way to verify, but it doesn't seem 100% essential. After all, it's not very common to add new calls to aminsert. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/