Skip site navigation (1) Skip section navigation (2)

Re: Patch for memory leaks in index scan

From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Dmitry Tkach <dmitry(at)openratings(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-bugs(at)postgresql(dot)org,pgsql-patches(at)postgresql(dot)org
Subject: Re: Patch for memory leaks in index scan
Date: 2002-06-20 21:25:58
Message-ID: 200206202125.g5KLPw019738@candle.pha.pa.us (view raw or flat)
Thread:
Lists: pgsql-bugspgsql-patches
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(at)openratings(dot)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(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman(at)candle(dot)pha(dot)pa(dot)us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

In response to

Responses

pgsql-bugs by date

Next:From: Tom LaneDate: 2002-06-20 21:28:22
Subject: Re: Patch for memory leaks in index scan
Previous:From: pgsql-bugsDate: 2002-06-20 18:16:06
Subject: Bug #694: can't store {digits} using JDBC

pgsql-patches by date

Next:From: Tom LaneDate: 2002-06-20 21:28:22
Subject: Re: Patch for memory leaks in index scan
Previous:From: Tom LaneDate: 2002-06-20 21:20:52
Subject: Re: Reduce heap tuple header size

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group