Feedback-ID: ia2694551:Fastmail
Date: Thu, 29 Feb 2024 13:20:39 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Alexander Lakhin <exclusion@gmail.com>,
	Soumyadeep Chakraborty <soumyadeep2007@gmail.com>,
 Matthias van de Meent <boekewurm+postgres@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
 Ashwin Agrawal <ashwinstar@gmail.com>
Subject: Re: brininsert optimization opportunity
Message-ID: <202402291220.gmjxa5wohzsh@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f412087a-6ef9-4b84-941a-e42f43150070@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202402291220.gmjxa5wohzsh%40alvherre.pgsql>
Precedence: bulk

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/