Re: Patch for memory leaks in index scan

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