Subject: Re: Should pg 11 use a lot more memory building an spgist index?
References: <20181026140927.oh4gd367st47li6h@alvherre.pgsql>
 <26004.1540563602@sss.pgh.pa.us> <20752.1540842517@sss.pgh.pa.us>
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Message-ID: <f7da983e-e489-3f05-5446-48812c8e35fc@lab.ntt.co.jp>
Date: Tue, 30 Oct 2018 19:10:31 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20752.1540842517@sss.pgh.pa.us>
Content-Type: multipart/mixed;
 boundary="------------5253696EAEF276D5B6588979"
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>, Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Alexander Korotkov <akorotkov@postgresql.org>,
        Bruno Wolff III <bruno@wolff.to>, pgsql-hackers@lists.postgresql.org
Precedence: bulk

This is a multi-part message in MIME format.
--------------5253696EAEF276D5B6588979
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit

On 2018/10/30 4:48, Tom Lane wrote:
> I wrote:
>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>> How about modifying SysScanDescData to have a memory context member,
>>> which is created by systable_beginscan and destroyed by endscan?
> 
>> I think it would still have problems, in that it would affect in which
>> context index_getnext delivers its output.  We could reasonably make
>> these sorts of changes in places where the entire index_beginscan/
>> index_getnext/index_endscan call structure is in one place, but that
>> doesn't apply for the systable functions.
> 
> Actually, after looking more closely, index_getnext doesn't return a
> palloc'd object anyway, or at least not one that the caller is responsible
> for managing.  So maybe that could work.

Instead of SysScanDescData, could we add one to IndexScanData?  It's
somewhat clear that catalog scans don't leak much, but user index scans
can, as seen in this case.

In this particular case, I see that it's IndexCheckExclusion() that
invokes check_unique_or_exclusion_constraint() repeatedly for each heap
tuple after finishing building index on the heap.  The latter performs
index_beginscan/endscan() for every heap tuple, but doesn't bother to
release the memory allocated for IndexScanDesc and its members.

I've tried to fix that with the attached patches.

0001 adds the ability for the callers of index_beginscan to specify a
memory context.  index_beginscan_internals switches to that context before
calling ambeginscan and stores into a new field xs_scan_cxt of
IndexScanData, but maybe storing it is unnecessary.

0002 builds on that and adds the ability for the callers of
check_exclusion_or_unique_constraints to specify a memory context.  Using
that, it fixes the leak by teaching IndexCheckExclusion and
ExecIndexInsertTuples to pass a memory context that's reset before calling
check_exclusion_or_unique_constraints() for the next tuple.

The following example shows that the patch works.

With HEAD:

create table foo (a int4range);
alter table foo add exclude using spgist (a with &&);
-- this consumes about 1GB
insert into foo select int4range(i,i+1) from generate_series(1, 100000) i;
alter table foo drop constraint foo_a_excl;
-- this too
alter table foo add exclude using spgist (a with &&);

Patched:

create table foo (a int4range);
alter table foo add exclude using spgist (a with &&);
-- memory consumption doesn't grow above 37MB
insert into foo select int4range(i,i+1) from generate_series(1, 100000) i;
alter table foo drop constraint foo_a_excl;
-- ditto
alter table foo add exclude using spgist (a with &&);

Thoughts?

Thanks,
Amit

--------------5253696EAEF276D5B6588979
Content-Type: text/plain; charset=UTF-8;
 name="v1-0001-Allow-IndexScanDesc-allocation-in-caller-specifie.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v1-0001-Allow-IndexScanDesc-allocation-in-caller-specifie.pa";
 filename*1="tch"