[geos-devel] An Immodest Proposal

[Paul Ramsey]

Mark, there's two issues at play here:

(1) - GEOS leaks, independently of the pgsql use of it, and yes, that
should be fixed
(2) - when you place a reference to a GEOS malloc'ed object in a
palloc'ed struct in pgsql, if the memorycontext where the struct
resides is cleaned up, you leak the GEOS object

It's (2) which seems to require a callback and hooks to do memory work
inside the pgsql pools. BTW, the XML support in PgSQL also uses
callbacks to do the memory allocation inside the pgsql pools, I have
found. However, as a core module, they get to create their own memory
context and have a piece of code right in core that cleans up their
context at the end of a transaction block.

If, alternately, the pgsql function manager provided a way to know
when fcinfo->flinfo->fn_mcxt was going to be freed, we could keep all
our GEOS objects in malloc'ed space and clean them up before the
context was freed.

So, I agree that (1) GEOS still needs work, I'm just not smart enough
to provide it, but I think the use of preparedgeometry is going to
require binding GEOS into the pgsql memory system in any event,
because of (2).

P.

On Wed, Oct 1, 2008 at 11:15 AM, Mark Cave-Ayland
<mark.cave-ayland at ilande.co.uk> wrote:
>
> On Mon, 2008-09-29 at 13:42 -0700, Paul Ramsey wrote:
>> All,
>>
>> We are currently grappling with a pretty big issue in the PostGIS/GEOS
>> integration, namely that PgSQL allocates it's memory inside pools
>> (palloc/pfree), while GEOS allocates in general memory (malloc/free).
>> This (a) imposes a requirement of memory management perfection on
>> GEOS, otherwise the long-running postgres processes will leak and (b)
>> can lead to insoluble problems in certain implementation patterns.
>>
>> We have recently found one such problem pattern. When using
>> preparedgeometry in postgis, we create a GEOS geometry and associated
>> prepared geometry, these are duly malloc'ed, but we store references
>> to them in a palloc'ed struct in a memory pool which lasts for the
>> life of a postgres query.  All is good (well, there are memory leaks
>> in the prepared stuff still, but those are theoretically fixable)
>> until the end of the query, when postgres cleans up the query memory
>> pool. All of a sudden the struct with the references to the geometry
>> and prepared geometry are gone -- but the objects have not been freed.
>>
>> Basically, the prepared geometry system will leak one geometry and one
>> prepared geometry for every SQL statement run, and there's SFA we can
>> do about it.
>>
>> My immodest proposal is to use geosInit as a location where users can
>> provide their own allocator/deallocators.  Right now, geosInit lets us
>> set custom error handlers, so this extends the idea further.
>>
>> In GEOS we would override new/delete in GEOS to our own functions that
>> would check and see if the custom alloc/dealloc functions were set. If
>> not, they would fall back to malloc/free, otherwise use what was
>> provided.  For postgis, we would provide palloc/pfree in our geosInit
>> calls.
>>
>> In this way, we could hook GEOS into the PgSQL pool system at runtime,
>> while still have it operate in the normal way when used with other
>> applications. And other applications with their own memory systems
>> could also more tightly integrate GEOS.
>>
>> Thoughts?
>>
>> P.
>
> Well, in terms of providing a quick solution that works with PostgreSQL
> then it can be seen as an improvement. But the fundamental issue is
> still that GEOS still leaks memory, and nearly every other application
> will still suffer the same consequences. Providing a hook so that other
> applications can handle the context tracking and lack of memory lifetime
> of a library which is known to leak is not really a suitable solution
> IMO.
>
> At the end of the day, for the solution to work 100% it still requires
> someone to go through the code and specify the lifetime for each "new"
> constructor in terms of a hierarchical memory system. I guess it depends
> on how desperately do people want a new GEOS release? I'd argue that
> there should be a new 3.0 branch release which has the containment fixes
> real soon now, but if there is time for 3.1, it would be well worth
> someone spending the time on this to nail the issue for once and for
> all. In terms of effort required, it would be more repetitive than
> technical.
>
>
> ATB,
>
> Mark.
>
>
>
>
> _______________________________________________
> geos-devel mailing list
> geos-devel at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/geos-devel
>