From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
cc: Bruno Wolff III <bruno@wolff.to>, pgsql-general@lists.postgresql.org,
        pgsql-hackers@lists.postgresql.org
Subject: Re: Should pg 11 use a lot more memory building an spgist index?
In-reply-to: <0e2b1baa-3d1c-7204-faa5-72c5d4a21a98@lab.ntt.co.jp>
References: <20181024012314.GA27428@wolff.to> <19003.1540370028@sss.pgh.pa.us>
 <20181024083748.GA3296@wolff.to> <19088.1540372871@sss.pgh.pa.us>
 <20181025174306.GA3184@wolff.to> <24288.1540545369@sss.pgh.pa.us>
 <0e2b1baa-3d1c-7204-faa5-72c5d4a21a98@lab.ntt.co.jp>
Comments: In-reply-to Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
	message dated "Fri, 26 Oct 2018 18:39:13 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <25594.1540547983.1@sss.pgh.pa.us>
Date: Fri, 26 Oct 2018 10:59:43 +0100
Message-ID: <25595.1540547983@sss.pgh.pa.us>
Precedence: bulk

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/10/26 18:16, Tom Lane wrote:
>> A quick review of the other index AM endscan methods seems to indicate
>> that they all try to clean up their mess.  So maybe we should just make
>> spgendscan do likewise.  Alternatively, we could decide that requiring
>> endscan methods to free storage is not very safe, in which case it would
>> be incumbent on check_exclusion_or_unique_constraint to make a temporary
>> context to run the scan in.  But if we're going to do the latter, I think
>> we oughta go full in and remove the retail pfree's from all the *endscan
>> functions.  We'd also have to review other callers of
>> index_beginscan/index_endscan to see which ones might also need their own
>> temp contexts.  So that would surely end up being more invasive than
>> just adding some pfree's to spgendscan would be.  Maybe in the long run
>> it'd be worth it, but probably not in the short run, or for back-patching.

> FWIW, I would prefer the latter.  Not that people write new AMs on a
> regular basis because we gave them an easier interface via CREATE ACCESS
> METHOD, but it still seems better for the core code to deal with memory
> allocation/freeing to avoid running into issues like this.

After a quick look around, I think that making systable_begin/endscan
do this is a nonstarter; there are just too many call sites that would
be affected.  Now, you could imagine specifying that indexes on system
catalogs (in practice, only btree) have to clean up at endscan time
but other index types don't, so that only operations that might be
scanning user indexes need to have suitable wrapping contexts.  Not sure
there's a lot of benefit to that, though.

			regards, tom lane