Re: Patch for memory leaks in index scan

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