Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix memory leak in repeated SPGIST index scans.

  2. Add support for nearest-neighbor (KNN) searches to SP-GiST

  1. Should pg 11 use a lot more memory building an spgist index?

    Bruno Wolff III <bruno@wolff.to> — 2018-10-24T01:23:14Z

    While reloading a database cluster to move from 10.5 to 11, I'm getting 
out of memory crashes that I did see when doing reloads on pg 10.
The statement flagged in the log is this:
2018-10-23 16:44:34.815 CDT [126839] STATEMENT:  ALTER TABLE ONLY public.iplocation
	    ADD CONSTRAINT overlap EXCLUDE USING spgist (network WITH &&);

iplocation has 4398722 rows.

This is geolite data where the networks are a partial covering of the total 
address spaces with no overlaps.

Should I expect to have to make config changes to make this work?

  2. Re: Should pg 11 use a lot more memory building an spgist index?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-24T08:33:48Z

    Bruno Wolff III <bruno@wolff.to> writes:
> While reloading a database cluster to move from 10.5 to 11, I'm getting 
> out of memory crashes that I did see when doing reloads on pg 10.
> The statement flagged in the log is this:
> 2018-10-23 16:44:34.815 CDT [126839] STATEMENT:  ALTER TABLE ONLY public.iplocation
> 	    ADD CONSTRAINT overlap EXCLUDE USING spgist (network WITH &&);

Hm, there's a fair amount of new code in SP-GIST in v11, so maybe you've
hit a memory leak in that.  Can you create a self-contained test case?

			regards, tom lane

  3. Re: Should pg 11 use a lot more memory building an spgist index?

    Bruno Wolff III <bruno@wolff.to> — 2018-10-24T08:37:48Z

    On Wed, Oct 24, 2018 at 09:33:48 +0100,
  Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Bruno Wolff III <bruno@wolff.to> writes:
>> While reloading a database cluster to move from 10.5 to 11, I'm getting
>> out of memory crashes that I did see when doing reloads on pg 10.
>> The statement flagged in the log is this:
>> 2018-10-23 16:44:34.815 CDT [126839] STATEMENT:  ALTER TABLE ONLY public.iplocation
>> 	    ADD CONSTRAINT overlap EXCLUDE USING spgist (network WITH &&);
>
>Hm, there's a fair amount of new code in SP-GIST in v11, so maybe you've
>hit a memory leak in that.  Can you create a self-contained test case?

I'll try. I think I should only need the geolite data to cause the problem 
and I can share that publicly. So far the problem seems to be happening 
consistently. I'll work on this at the office, but probably won't get it 
done until the afternoon.

If I have a substantial database dump file to provide for reproducing this 
do you prefer it on a web server somewhere? I expect that mailing very 
large attachments to the lists is a bad idea.

  4. Re: Should pg 11 use a lot more memory building an spgist index?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-24T09:21:11Z

    Bruno Wolff III <bruno@wolff.to> writes:
> If I have a substantial database dump file to provide for reproducing this 
> do you prefer it on a web server somewhere? I expect that mailing very 
> large attachments to the lists is a bad idea.

No, don't do that.  If you can make sample data available for download,
or point to some accessible dataset somewhere, that'd work.

			regards, tom lane

  5. Re: Should pg 11 use a lot more memory building an spgist index?

    Bruno Wolff III <bruno@wolff.to> — 2018-10-25T02:41:44Z

    On Tue, Oct 23, 2018 at 20:23:14 -0500,
  Bruno Wolff III <bruno@wolff.to> wrote:
>While reloading a database cluster to move from 10.5 to 11, I'm 
>getting out of memory crashes that I did see when doing reloads on pg 
>10.
>The statement flagged in the log is this:
>2018-10-23 16:44:34.815 CDT [126839] STATEMENT:  ALTER TABLE ONLY public.iplocation
>	    ADD CONSTRAINT overlap EXCLUDE USING spgist (network WITH &&);
>
>iplocation has 4398722 rows.

I'm trying to reproduce this on my desktop but so far it isn't producing 
the same issue. The database is still loading after 9 hours, but it looks 
like it got past the point where the problem index was created. (I'm not 
sure how to check if the index has really finished being created, but \d 
shows it as existing.)

I'll know better tomorrow.

My workstation is using the Fedora version of postgresql 11 which might 
have some relevant difference from the pgdg version for rhel7. I still 
have a number of things I can try, but they might take significant time 
to run and I might not get a reasonable test case for a while.

  6. Re: Should pg 11 use a lot more memory building an spgist index?

    Bruno Wolff III <bruno@wolff.to> — 2018-10-25T12:10:29Z

    It looks like it got past creating the exclude constraint based on the 
ordering of commands in the dump file. However creating a more normal 
spgist index is taking a very long time with a lot of disk wait time. 
CPU usage seems pretty low for the amount of time it has been working 
on building that index, but maybe that is normal for building indexes.
I used the almost the default postgresql.conf on my workstation. I bumped 
up the limits in a few places on the server that could have allowed a lot 
more memory to be used especially if the index creation was parallelized. 
While the load is running I'll see if I can tell if there is a memory leak 
with this index build. Once it finishes, I can dump a specific table 
and test building the exclude spgist index with some different settings 
to see if I can reproduce the out of memory error with a lot less data 
then is in the whole database.

  7. Re: Should pg 11 use a lot more memory building an spgist index?

    Bruno Wolff III <bruno@wolff.to> — 2018-10-25T17:43:06Z

    On Wed, Oct 24, 2018 at 10:21:11 +0100,
  Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Bruno Wolff III <bruno@wolff.to> writes:
>> If I have a substantial database dump file to provide for reproducing this
>> do you prefer it on a web server somewhere? I expect that mailing very
>> large attachments to the lists is a bad idea.
>
>No, don't do that.  If you can make sample data available for download,
>or point to some accessible dataset somewhere, that'd work.
>
>			regards, tom lane

I have something that seems to produce it on rhel7. Fedora isn't working 
well either, but the difference may be due to postgresql.conf being 
different or some difference in the Fedora build.

http://wolff.to/iplocation is a bit under 400mb. It should download at about 
1MB/sec. It is a plain text dump of the iplocation table with the alter table 
for the constaint / exclude index added at the end.

http://wolff.to/postgresql.conf is the config file I use on the server.

The server has the following installed (but you don't need plperl for the 
test):
postgresql11-plperl-11.0-1PGDG.rhel7.x86_64
postgresql11-libs-11.0-1PGDG.rhel7.x86_64
postgresql11-docs-11.0-1PGDG.rhel7.x86_64
postgresql11-11.0-1PGDG.rhel7.x86_64
postgresql11-server-11.0-1PGDG.rhel7.x86_64

The output of
createdb -U postgres test
psql -U postgres -f iplocation test
is:
SET
SET
SET
SET
SET
 set_config 
------------
 
(1 row)

SET
SET
SET
SET
SET
CREATE TABLE
ALTER TABLE
COPY 4398722
psql:iplocation:4398789: ERROR:  out of memory
DETAIL:  Failed on request of size 6264 in memory context "PortalContext".

It is certainly possible that my postgresql.conf file is bad and that I 
just got away with it under 10.5 by the. The server is a vm with 32GB of 
memory allocated to it. I set vm.overcommit_memory = 2 to avoid the oom 
killer after upgrading to 11. Before that I didn't have a problem.

On Fedora with a more vanilla postgresql.conf the exclude constraint 
built fine, but creating an spgist index file is taking forever (near a 
day now) creating a normal spgist index on an ip address column for a 
table with a lot of rows (I think around 150 million), that ran in a 
reasonable amount of time on the server.

  8. Re: Should pg 11 use a lot more memory building an spgist index?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-26T09:16:09Z

    Bruno Wolff III <bruno@wolff.to> writes:
> I have something that seems to produce it on rhel7. Fedora isn't working 
> well either, but the difference may be due to postgresql.conf being 
> different or some difference in the Fedora build.

Hmm, in my hands this produces the same size leak (~28GB) in either v10
or v11.  In HEAD, somebody's made it even worse (~43GB).  So this is
certainly pretty broken, but I'm not sure why it seems worse to you in
v11 than before.

The core of the problem I see is that check_exclusion_or_unique_constraint
does index_beginscan/index_rescan/index_endscan in a long-lived context,
while spgendscan seems to have employed dice while deciding which of
spgbeginscan's allocations it would bother to pfree.  This is ancient,
though the specific case you have here can only be tested back to v10
because the inet SPGIST opclass wasn't there before.

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.

Thoughts?

			regards, tom lane

  9. Re: Should pg 11 use a lot more memory building an spgist index?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-26T09:39:13Z

    On 2018/10/26 18:16, Tom Lane wrote:
> The core of the problem I see is that check_exclusion_or_unique_constraint
> does index_beginscan/index_rescan/index_endscan in a long-lived context,
> while spgendscan seems to have employed dice while deciding which of
> spgbeginscan's allocations it would bother to pfree.  This is ancient,
> though the specific case you have here can only be tested back to v10
> because the inet SPGIST opclass wasn't there before.
> 
> 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.

Thanks,
Amit

  10. Re: Should pg 11 use a lot more memory building an spgist index?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-26T09:59:43Z

    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

  11. Re: Should pg 11 use a lot more memory building an spgist index?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-26T10:08:44Z

    On 2018/10/26 18:59, Tom Lane wrote:
> 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.

By "core code", I didn't mean systable_being/endscan, but rather
check_exclusion_or_unique_constraint() or its core-side caller(s).  But
maybe I misunderstood something about your diagnosis upthread where you said:

"The core of the problem I see is that check_exclusion_or_unique_constraint
does index_beginscan/index_rescan/index_endscan in a long-lived context,"

Thanks,
Amit

  12. Re: Should pg 11 use a lot more memory building an spgist index?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-26T10:20:13Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2018/10/26 18:59, Tom Lane wrote:
>> 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.

> By "core code", I didn't mean systable_being/endscan, but rather
> check_exclusion_or_unique_constraint() or its core-side caller(s).

Well, we'd need to consider any call path leading to index_endscan.
One of those is systable_endscan and its multitude of callers.  It seems
unlikely that you could just switch context in systable_beginscan
without breaking many of the callers.

If we forbade leaks in system catalog index AMs, then the number of
places that would need work would be manageable (about 3, it looked
like).  But then it seems more like a wart than a real API improvement.

			regards, tom lane

  13. Re: Should pg 11 use a lot more memory building an spgist index?

    Bruno Wolff III <bruno@wolff.to> — 2018-10-26T11:23:14Z

    On Fri, Oct 26, 2018 at 10:16:09 +0100,
  Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Bruno Wolff III <bruno@wolff.to> writes:
>> I have something that seems to produce it on rhel7. Fedora isn't working
>> well either, but the difference may be due to postgresql.conf being
>> different or some difference in the Fedora build.
>
>Hmm, in my hands this produces the same size leak (~28GB) in either v10
>or v11.  In HEAD, somebody's made it even worse (~43GB).  So this is
>certainly pretty broken, but I'm not sure why it seems worse to you in
>v11 than before.

As a short term work around, could I create the index first and use 
insert statements, each in their own transaction, to get the table loaded 
with the index?

Is the issue on Fedora taking very long to build a normal spgist index for 
network addresses worth pursuing separately, or is it likely to be the same 
underlying cause? I don't really need to get this working there, as that was 
just to help with testing.

I could also try adjusting memory limits temporarily. If the leak is 28GB 
on a 32GB system I might be able to get the index built if less memory 
is tied up in other things. My workstation also has 32GB and the exclude 
index did build there with the postgresql.conf having lower memory limits.

  14. Re: Should pg 11 use a lot more memory building an spgist index?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-26T12:44:07Z

    Bruno Wolff III <bruno@wolff.to> writes:
>   Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hmm, in my hands this produces the same size leak (~28GB) in either v10
>> or v11.  In HEAD, somebody's made it even worse (~43GB).  So this is
>> certainly pretty broken, but I'm not sure why it seems worse to you in
>> v11 than before.

> As a short term work around, could I create the index first and use 
> insert statements, each in their own transaction, to get the table loaded 
> with the index?

Yes; it might also be that you don't even need to break it up into
separate statements.

> Is the issue on Fedora taking very long to build a normal spgist index for 
> network addresses worth pursuing separately, or is it likely to be the same 
> underlying cause?

This issue only applies if it was an exclusion constraint.  If you saw
slowness or bloat with a plain index, that would be worth investigating
separately.

			regards, tom lane

  15. Re: Should pg 11 use a lot more memory building an spgist index?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-10-26T14:09:27Z

    On 2018-Oct-26, Tom Lane wrote:

> 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.

How about modifying SysScanDescData to have a memory context member,
which is created by systable_beginscan and destroyed by endscan?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  16. Re: Should pg 11 use a lot more memory building an spgist index?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-26T14:20:02Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Oct-26, Tom Lane wrote:
>> 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.

> 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.

Also, going in that direction would imply an additional memory context
switch / switch-back per tuple processed (around the index_getnext call),
which would create questions about whether it has a negative performance
impact.

			regards, tom lane

  17. Re: Should pg 11 use a lot more memory building an spgist index?

    Bruno Wolff III <bruno@wolff.to> — 2018-10-26T14:54:37Z

    On Fri, Oct 26, 2018 at 13:44:07 +0100,
  Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Bruno Wolff III <bruno@wolff.to> writes:
>
>> As a short term work around, could I create the index first and use
>> insert statements, each in their own transaction, to get the table loaded
>> with the index?
>
>Yes; it might also be that you don't even need to break it up into
>separate statements.

It was time to refresh the geolite data anyway so I tried this. I needed 
to turn memory_overcommit back on (0) to avoid an error, but the load went OK 
without the oom killer doing anything. So things are fully working again.

Thanks for your help.

>> Is the issue on Fedora taking very long to build a normal spgist index for
>> network addresses worth pursuing separately, or is it likely to be the same
>> underlying cause?
>
>This issue only applies if it was an exclusion constraint.  If you saw
>slowness or bloat with a plain index, that would be worth investigating
>separately.

I'll start a seperate thread if I get something to reasonably ask about. 
The current dataset is probably a lot larger then needed to demonstrate the 
issue. The difference might be do to configuration or how Fedora built it. 
And I'll want to compare back to version 10. In the end I'll probably ask 
why it is slower in one case as opposed to the other and it might not even 
be a real bug.

  18. Re: Should pg 11 use a lot more memory building an spgist index?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-29T19:48:37Z

    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.

I was confused about why the memory leak in Bruno's example is so much
larger in HEAD than v11; spgbeginscan does allocate more stuff than
before, but only if numberOfOrderBys > 0, which surely doesn't apply for
the exclusion-check code path.  Eventually I realized that the difference
is because commit 2a6368343 made SpGistScanOpaqueData a good deal larger
than it had been (10080 vs 6264 bytes, on my x86_64 box), so it was just
the failure to pfree that struct that accounted for the bigger leak.

There's another issue with 2a6368343, though: it added a couple of
fmgr_info_copy calls to spgbeginscan.  I don't understand why it'd be a
good idea to do that rather than using the FmgrInfos in the index's
relcache entry directly.  Making a copy every time risks a memory leak,
because spgendscan has no way to free any fn_extra data that gets
allocated by the called function; and by the same token it's inefficient,
because the function has no way to cache fn_extra data across queries.

If we consider only the leak aspect, this coding presents a reason why
we should try to change things as Alvaro suggests.  However, because of
the poor-caching aspect, I think this code is pretty broken anyway,
and once we fix it the issue is much less clear-cut.

(Looking around, it seems like we're very schizophrenic about whether
to copy index relcache support function FmgrInfos or just use them
directly.  But nbtree and hash seem to always do the latter, so I think
it's probably reasonable to standardize on that.)

Anyway, the attached proposed patch for HEAD makes Bruno's problem go
away, and it also fixes an unrelated leak introduced by 2a6368343
because it reallocates so->orderByTypes each time through spgrescan.
I think we should apply this, and suitable subsets in the back branches,
to fix the immediate problem.  Then we can work at leisure on a more
invasive HEAD-only patch, if anyone is excited about that.

			regards, tom lane

  19. Re: Should pg 11 use a lot more memory building an spgist index?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-30T07:30:22Z

    On 2018/10/30 4:48, Tom Lane wrote:
> I was confused about why the memory leak in Bruno's example is so much
> larger in HEAD than v11; spgbeginscan does allocate more stuff than
> before, but only if numberOfOrderBys > 0, which surely doesn't apply for
> the exclusion-check code path.  Eventually I realized that the difference
> is because commit 2a6368343 made SpGistScanOpaqueData a good deal larger
> than it had been (10080 vs 6264 bytes, on my x86_64 box), so it was just
> the failure to pfree that struct that accounted for the bigger leak.
> 
> There's another issue with 2a6368343, though: it added a couple of
> fmgr_info_copy calls to spgbeginscan.  I don't understand why it'd be a
> good idea to do that rather than using the FmgrInfos in the index's
> relcache entry directly.  Making a copy every time risks a memory leak,
> because spgendscan has no way to free any fn_extra data that gets
> allocated by the called function; and by the same token it's inefficient,
> because the function has no way to cache fn_extra data across queries.
> 
> If we consider only the leak aspect, this coding presents a reason why
> we should try to change things as Alvaro suggests.  However, because of
> the poor-caching aspect, I think this code is pretty broken anyway,
> and once we fix it the issue is much less clear-cut.
> 
> (Looking around, it seems like we're very schizophrenic about whether
> to copy index relcache support function FmgrInfos or just use them
> directly.  But nbtree and hash seem to always do the latter, so I think
> it's probably reasonable to standardize on that.)

Agreed about trying to avoid fmgr_info_copy(), at least in this case.

By the way, do I get it right that the reason to want to avoid copying in
this instance is that the currently active context could be a long-lived
one, as in the case of create index, alter table add constraint, etc.
calling the copying code for every tuple?  There are other instances of
fmgr_info_copy throughout index AM implementations, including the helper
function ScanKeyEntryInitializeWithInfo() called from them, but the copies
made in those cases don't appear very leak-prone.

> Anyway, the attached proposed patch for HEAD makes Bruno's problem go
> away, and it also fixes an unrelated leak introduced by 2a6368343
> because it reallocates so->orderByTypes each time through spgrescan.
> I think we should apply this, and suitable subsets in the back branches,
> to fix the immediate problem.  Then we can work at leisure on a more
> invasive HEAD-only patch, if anyone is excited about that.

Makes sense to fix it like this for back-patching.

Thanks,
Amit

  20. Re: Should pg 11 use a lot more memory building an spgist index?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-30T10:10:31Z

    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

  21. Re: Should pg 11 use a lot more memory building an spgist index?

    amit <amitlangote09@gmail.com> — 2018-10-30T12:27:42Z

    On Tue, Oct 30, 2018 at 7:11 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> 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 &&);

Sorry I forgot to try the example with your patch.  Maybe, it will
have more or less the same behavior as mine, although I didn't realize
that when I started writing my patch.

Thanks,
Amit

  22. Re: Should pg 11 use a lot more memory building an spgist index?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-31T01:50:19Z

    On 2018/10/30 21:27, Amit Langote wrote:
> On Tue, Oct 30, 2018 at 7:11 PM Amit Langote
>> 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 &&);
> 
> Sorry I forgot to try the example with your patch.  Maybe, it will
> have more or less the same behavior as mine, although I didn't realize
> that when I started writing my patch.

Yep, I checked that fix-spgist-memory-leaks-1.patch posted upthread gives
almost the same numbers, i.e., the maximum amount of memory consumed.

Maybe, we don't need to spoil the interface of index_beginscan with the
new memory context argument like my patch does if the simple following of
its contract by amendscan would suffice.

Thanks,
Amit

  23. Re: Should pg 11 use a lot more memory building an spgist index?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-31T21:21:29Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> Maybe, we don't need to spoil the interface of index_beginscan with the
> new memory context argument like my patch does if the simple following of
> its contract by amendscan would suffice.

Yeah, I'm not enamored of changing the API of index_beginscan for this;
the existing convention that it should allocate in CurrentMemoryContext
seems perfectly suitable.  And changing it would create a lot of code
churn, not only for us but for externally-maintained AMs.

What is at stake is changing the API of amendscan, to specify that it
need not release memory because the current context is expected to be
destroyed or reset shortly after ending the scan.  Then, for the small
number of call sites where that wouldn't be true, it's on those callers
to set up a suitable context and switch into it.  Note this is actually
forwards compatible, in that an AM that's still following the convention
of releasing stuff manually would not be broken.

			regards, tom lane

  24. Re: Should pg 11 use a lot more memory building an spgist index?

    Bruno Wolff III <bruno@wolff.to> — 2018-11-02T21:01:34Z

    I see that a fix got committed. Thanks!
I'll double check it after the point release comes out (which looks like it 
will be next week) and let you know if there is still a problem.