Thread: Patch for memory leaks in index scan
Ok, this sit around for a while, and, because there was no responses, I assume, that nothing jumps out
at you as being terribly with my logic...
Here is the patch (see the original problem description in the bottom)... It seems to be working (at least that query,
that used to be running out of memory is now able to finish).
*** nodeIndexscan.c.orig Fri Apr 19 10:29:57 2002
--- nodeIndexscan.c Fri Apr 19 10:30:00 2002
***************
*** 505,510 ****
--- 505,514 ----
*/
ExecClearTuple(scanstate->cstate.cs_ResultTupleSlot);
ExecClearTuple(scanstate->css_ScanTupleSlot);
+ pfree(scanstate);
+ pfree(indexstate->iss_RelationDescs);
+ pfree(indexstate->iss_ScanDescs);
+ pfree(indexstate);
}
/* ----------------------------------------------------------------
I hope, it helps....
Dima
-------- Original Message --------
It seems that ExecInit/EndIndexScan is leaking some memory...
For example, if I run a query, that uses an index scan, and call MemoryContextStats (CurrentMemoryContext) before
ExecutorStart() and after ExecutorEnd() in ProcessQuery(), I am consistently seeing that the 'after' call shows
256 bytes more used, then 'before'...
In many common cases, this is not a problem, because pg_exec_query_string () will call
MemoryContextResetAndDeleteChildren(), that will clean everything up eventually...
But it still seems to cause problems, when the index scan is not directly invoked from pg_exec_query_string ().
For example:
create table a
(
id int primary key,
data char(100)
);
create table b
(
id int references a,
more_data char(100)
);
create function merge_data (int,data) returns char(200) as 'select data || $2 from a where id = $1;' language 'sql';
Now, if b is large enough, then something like:
select merge_data (id,more_data) from b;
Will eventually run out of memory - it will loose 256 bytes after each row, or about a GIG after 4 million rows...
The problem seems to be in ExecEndIndexScan - it does not release scanstate, indexstate, indexstate->iss_RelationDescs
and indexstate -> iss_ScanDescs...
I am not familiar enough with the core code, to just jump in and start fixing it, but I can make the patch,
test it and send it to you, if you guys let me know if what I am saying makes sense to you...
Or am I missing something here?
Thanks!
Dima
Dmitry Tkach <dmitry@openratings.com> writes:
> *** nodeIndexscan.c.orig Fri Apr 19 10:29:57 2002
> --- nodeIndexscan.c Fri Apr 19 10:30:00 2002
> ***************
> *** 505,510 ****
> --- 505,514 ----
> */
> ExecClearTuple(scanstate->cstate.cs_ResultTupleSlot);
> ExecClearTuple(scanstate->css_ScanTupleSlot);
> + pfree(scanstate);
> + pfree(indexstate->iss_RelationDescs);
> + pfree(indexstate->iss_ScanDescs);
> + pfree(indexstate);
> }
I do not believe this patch will fix anything.
In general, the EndNode routines do not bother with releasing memory,
because it's the end of the query and we're about to drop or reset
the per-query context anyway. If the above pfrees are actually needed
then there are a heck of a lot of other places that need explicit
pfrees. And that is not a path to a solution, because there will
*always* be places that forgot a pfree. What's needed is a structural
solution.
I think your real complaint is that SQL functions leak memory. They
should be creating a sub-context for their queries that can be freed
when the function finishes.
regards, tom lane
Tom Lane wrote: >Dmitry Tkach <dmitry@openratings.com> writes: > >>*** nodeIndexscan.c.orig Fri Apr 19 10:29:57 2002 >>--- nodeIndexscan.c Fri Apr 19 10:30:00 2002 >>*************** >>*** 505,510 **** >>--- 505,514 ---- >> */ >> ExecClearTuple(scanstate->cstate.cs_ResultTupleSlot); >> ExecClearTuple(scanstate->css_ScanTupleSlot); >>+ pfree(scanstate); >>+ pfree(indexstate->iss_RelationDescs); >>+ pfree(indexstate->iss_ScanDescs); >>+ pfree(indexstate); >> } >> > >I do not believe this patch will fix anything. > Well... It does fix the query I mentioned in my first message (and a few others I was doing when I ran into this problem) > >In general, the EndNode routines do not bother with releasing memory, > Well... Not quite true. For example, ExecEndIndexScan () does release lots of stuff, that was allocated in ExecInitIndexScan (), it just forgets about these four things... > >because it's the end of the query and we're about to drop or reset >the per-query context anyway. > ... if the query manages to complete without running out of memory like in my case :-) > If the above pfrees are actually needed >then there are a heck of a lot of other places that need explicit >pfrees. > Perhaps... I haven't run into any other places just yet... >And that is not a path to a solution, because there will >*always* be places that forgot a pfree. > Sure :-) You can say this about any bug pretty much :-) There will *always* be bugs... That doesn't mean that the ones, that are found should not be fixed though > What's needed is a structural >solution. > I understand what you are saying... I was looking around for one for a while, but it seems, but there doesn't seem to be anything 'more structural' for this particular case, as far as I can see - per query context does get released properly in the end, it's just that the query requires too much temporary memory to complete, so the end is actually never reached... After all, I repeat, ExecEndIndexScan () DOES free up lots of memory, so I don't see how adding these four things that got forgotten would hurt. > >I think your real complaint is that SQL functions leak memory. They >should be creating a sub-context for their queries that can be freed >when the function finishes. > I guess, you are right here - I tried using a subquery instead of a function, and that is not leaking, because it does ExecRescan () for each outer tuple, instead of reinitializing the executor completely as it is the case with sql func . fmgr_sql () DOES use it's own context, but it looks like it doesn't want to reset it every time, because of caching... Perhaps, it could just execute every command in a function as a SubPlan, instead of reinitializing every time, but that wouldn't be a simple 4 line fix, that certainly doesn't break anything that was working before :-) I was thinking in terms of a quick PATCH, that can fix a particular problem, and would be safe to be put in. It does not prevent any "structural solution" from being implemented if somebody comes up with one in the future, and it would STILL be useful even when then solution is implemented, because it makes memory usage more conservative, thus reducing the load on system resources... Frankly, I don't see what is your problem with it at all :-) Dima
Dmitry Tkach <dmitry@openratings.com> writes:
> Frankly, I don't see what is your problem with it at all :-)
Mainly, I don't like wasting cycles and code space on partial solutions
;-). There's certainly no reason you shouldn't make this patch locally
if the leak is getting in your way, but for an official solution I'd
rather see us make a systemic fix --- and then we could rip out the
retail pfree's both here and in other EndNode calls.
regards, tom lane
I am dumping a rather large PostgreSQL database with 895,000 rows or so.
running 'pg_dump -d databse -u --table=tbl -f ./tbl.sql' makes a 425MB file.
The problem is that whether the database is remote or local [i have tried
running it on the DB server itself with the same result], it takes up a good
half gigabyte of RAM for the course of the dump. Why does it load all of
this into memory on the client machine rather than output it as it comes in.
On one server, this has caused the machine to swap out a good 250MB to disk,
running up the system load to very high numbers. The database server seems
to serve it as any other request, but the client server that is running
pg_dump seems to be struggling.
My main question is not that it does it but why this is how it is done?
Isn't it more efficient to dump it out to the file or STDOUT (if -f isn't
specified) rather than load the entire result set into memory? It's not
sorting it or anything.
Any help would be appreciated.
--
Mike
"Mike Rogers" <temp6453@hotmail.com> writes:
> The problem is that whether the database is remote or local [i have tried
> running it on the DB server itself with the same result], it takes up a good
> half gigabyte of RAM for the course of the dump. Why does it load all of
> this into memory on the client machine rather than output it as it comes in.
Use a more recent pg_dump. This is fixed in 7.2.
regards, tom lane
With no solution on the horizon, and the author saying it fixes some of his trigger queries, I am inclined to apply this. I don't see any downside except for some extra pfrees if we ever fix this. --------------------------------------------------------------------------- Dmitry Tkach wrote: > Tom Lane wrote: > > >Dmitry Tkach <dmitry@openratings.com> writes: > > > >>*** nodeIndexscan.c.orig Fri Apr 19 10:29:57 2002 > >>--- nodeIndexscan.c Fri Apr 19 10:30:00 2002 > >>*************** > >>*** 505,510 **** > >>--- 505,514 ---- > >> */ > >> ExecClearTuple(scanstate->cstate.cs_ResultTupleSlot); > >> ExecClearTuple(scanstate->css_ScanTupleSlot); > >>+ pfree(scanstate); > >>+ pfree(indexstate->iss_RelationDescs); > >>+ pfree(indexstate->iss_ScanDescs); > >>+ pfree(indexstate); > >> } > >> > > > >I do not believe this patch will fix anything. > > > Well... It does fix the query I mentioned in my first message (and a few > others I was doing when I ran into this problem) > > > > >In general, the EndNode routines do not bother with releasing memory, > > > Well... Not quite true. For example, ExecEndIndexScan () does release > lots of stuff, that was allocated in ExecInitIndexScan (), it just > forgets about these four things... > > > > >because it's the end of the query and we're about to drop or reset > >the per-query context anyway. > > > ... if the query manages to complete without running out of memory like > in my case :-) > > > If the above pfrees are actually needed > >then there are a heck of a lot of other places that need explicit > >pfrees. > > > Perhaps... I haven't run into any other places just yet... > > >And that is not a path to a solution, because there will > >*always* be places that forgot a pfree. > > > Sure :-) > You can say this about any bug pretty much :-) There will *always* be > bugs... That doesn't mean that the ones, that are found should not be > fixed though > > > What's needed is a structural > >solution. > > > I understand what you are saying... I was looking around for one for a > while, but it seems, but there doesn't seem > to be anything 'more structural' for this particular case, as far as I > can see - per query context does get released properly in the end, it's > just that the query requires too much temporary memory to complete, so > the end is actually never reached... After all, I repeat, > ExecEndIndexScan () DOES free up lots of memory, so I don't see how > adding these four things that got forgotten would hurt. > > > > >I think your real complaint is that SQL functions leak memory. They > >should be creating a sub-context for their queries that can be freed > >when the function finishes. > > > > I guess, you are right here - I tried using a subquery instead of a > function, and that is not leaking, because it does > ExecRescan () for each outer tuple, instead of reinitializing the > executor completely as it is the case with sql func . > > fmgr_sql () DOES use it's own context, but it looks like it doesn't want > to reset it every time, because of caching... > > Perhaps, it could just execute every command in a function as a SubPlan, > instead of reinitializing every time, but that wouldn't be a simple 4 > line fix, that certainly doesn't break anything that was working before :-) > > I was thinking in terms of a quick PATCH, that can fix a particular > problem, and would be safe to be put in. > > It does not prevent any "structural solution" from being implemented if > somebody comes up with one in the future, and it would STILL be useful > even when then solution is implemented, because it makes memory usage > more conservative, thus reducing the load on system resources... > > Frankly, I don't see what is your problem with it at all :-) > > Dima > > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 3: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > With no solution on the horizon, and the author saying it fixes some of > > his trigger queries, I am inclined to apply this. I don't see any > > downside except for some extra pfrees if we ever fix this. > > Sure, apply away. I was mainly expressing my angst over the lack of > a system-wide solution. Sure, many of the patches I do exist for only a few releases, but they meet users needs until a better solution comes along. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> With no solution on the horizon, and the author saying it fixes some of
> his trigger queries, I am inclined to apply this. I don't see any
> downside except for some extra pfrees if we ever fix this.
Sure, apply away. I was mainly expressing my angst over the lack of
a system-wide solution.
regards, tom lane
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Dmitry Tkach wrote:
> Ok, this sit around for a while, and, because there was no responses, I assume, that nothing jumps out
> at you as being terribly with my logic...
> Here is the patch (see the original problem description in the bottom)... It seems to be working (at least that
query,
> that used to be running out of memory is now able to finish).
>
> *** nodeIndexscan.c.orig Fri Apr 19 10:29:57 2002
> --- nodeIndexscan.c Fri Apr 19 10:30:00 2002
> ***************
> *** 505,510 ****
> --- 505,514 ----
> */
> ExecClearTuple(scanstate->cstate.cs_ResultTupleSlot);
> ExecClearTuple(scanstate->css_ScanTupleSlot);
> + pfree(scanstate);
> + pfree(indexstate->iss_RelationDescs);
> + pfree(indexstate->iss_ScanDescs);
> + pfree(indexstate);
> }
>
> /* ----------------------------------------------------------------
>
> I hope, it helps....
>
> Dima
>
>
> -------- Original Message --------
>
> It seems that ExecInit/EndIndexScan is leaking some memory...
>
> For example, if I run a query, that uses an index scan, and call MemoryContextStats (CurrentMemoryContext) before
> ExecutorStart() and after ExecutorEnd() in ProcessQuery(), I am consistently seeing that the 'after' call shows
> 256 bytes more used, then 'before'...
>
> In many common cases, this is not a problem, because pg_exec_query_string () will call
> MemoryContextResetAndDeleteChildren(), that will clean everything up eventually...
>
> But it still seems to cause problems, when the index scan is not directly invoked from pg_exec_query_string ().
> For example:
>
> create table a
> (
> id int primary key,
> data char(100)
> );
>
> create table b
> (
> id int references a,
> more_data char(100)
> );
>
> create function merge_data (int,data) returns char(200) as 'select data || $2 from a where id = $1;' language 'sql';
>
> Now, if b is large enough, then something like:
>
> select merge_data (id,more_data) from b;
>
> Will eventually run out of memory - it will loose 256 bytes after each row, or about a GIG after 4 million rows...
>
> The problem seems to be in ExecEndIndexScan - it does not release scanstate, indexstate,
indexstate->iss_RelationDescs
> and indexstate -> iss_ScanDescs...
>
> I am not familiar enough with the core code, to just jump in and start fixing it, but I can make the patch,
> test it and send it to you, if you guys let me know if what I am saying makes sense to you...
> Or am I missing something here?
>
> Thanks!
>
> Dima
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Patch applied. Thanks. --------------------------------------------------------------------------- Dmitry Tkach wrote: > Ok, this sit around for a while, and, because there was no responses, I assume, that nothing jumps out > at you as being terribly with my logic... > Here is the patch (see the original problem description in the bottom)... It seems to be working (at least that query, > that used to be running out of memory is now able to finish). > > *** nodeIndexscan.c.orig Fri Apr 19 10:29:57 2002 > --- nodeIndexscan.c Fri Apr 19 10:30:00 2002 > *************** > *** 505,510 **** > --- 505,514 ---- > */ > ExecClearTuple(scanstate->cstate.cs_ResultTupleSlot); > ExecClearTuple(scanstate->css_ScanTupleSlot); > + pfree(scanstate); > + pfree(indexstate->iss_RelationDescs); > + pfree(indexstate->iss_ScanDescs); > + pfree(indexstate); > } > > /* ---------------------------------------------------------------- > > I hope, it helps.... > > Dima > > > -------- Original Message -------- > > It seems that ExecInit/EndIndexScan is leaking some memory... > > For example, if I run a query, that uses an index scan, and call MemoryContextStats (CurrentMemoryContext) before > ExecutorStart() and after ExecutorEnd() in ProcessQuery(), I am consistently seeing that the 'after' call shows > 256 bytes more used, then 'before'... > > In many common cases, this is not a problem, because pg_exec_query_string () will call > MemoryContextResetAndDeleteChildren(), that will clean everything up eventually... > > But it still seems to cause problems, when the index scan is not directly invoked from pg_exec_query_string (). > For example: > > create table a > ( > id int primary key, > data char(100) > ); > > create table b > ( > id int references a, > more_data char(100) > ); > > create function merge_data (int,data) returns char(200) as 'select data || $2 from a where id = $1;' language 'sql'; > > Now, if b is large enough, then something like: > > select merge_data (id,more_data) from b; > > Will eventually run out of memory - it will loose 256 bytes after each row, or about a GIG after 4 million rows... > > The problem seems to be in ExecEndIndexScan - it does not release scanstate, indexstate, indexstate->iss_RelationDescs > and indexstate -> iss_ScanDescs... > > I am not familiar enough with the core code, to just jump in and start fixing it, but I can make the patch, > test it and send it to you, if you guys let me know if what I am saying makes sense to you... > Or am I missing something here? > > Thanks! > > Dima > > > > ---------------------------(end of broadcast)--------------------------- > TIP 3: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026