leaks in TopMemoryContext?

From: Neil Conway <neilc(at)samurai(dot)com>
To: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: leaks in TopMemoryContext?
Date: 2006-01-11 07:45:37
Message-ID: 1136965537.9143.71.camel@localhost.localdomain
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

While thinking about how to do memory management for the TupleDesc
refcounting changes, it occurred to me that this coding pattern is
dangerous:

local_var = MemoryContextAlloc(TopMemoryContext, ...);
func_call();
/* ... */
/* update global state */
if (global != NULL)
pfree(global);
global = local_var;

the idea being that we're allocating some memory that should live longer
than the current transaction. For example, see circa line 141 in
mb/mbutils.c:SetClientEncoding().

This works fine if no error occurs: presumably, the code takes care of
releasing the allocation in TopMemoryContext when appropriate (as
mbutils.c does). The problem arises if there is an error: suppose that
func_call() in the example above elogs. The elog handler will abort the
current transaction and reset per-transaction memory contexts, but
TopMemoryContext will not be reset. Therefore the memory allocated for
"local_var" will be leaked.

To see the leak in practice, insert an elog(ERROR) in mbutils.c here:

line 141:
oldcontext = MemoryContextSwitchTo(TopMemoryContext);
to_server = palloc(sizeof(FmgrInfo));
to_client = palloc(sizeof(FmgrInfo));
fmgr_info(to_server_proc, to_server);
fmgr_info(to_client_proc, to_client);
>>>>> elog(ERROR, "...");
MemoryContextSwitchTo(oldcontext);

Running a stream of "SET client_encoding = 'koi8';" on my machine yields
a steady growth in the size of the TopMemoryContext.[1]

The elog(ERROR) above is inserted by hand to make the problem easier to
reproduce, but it is certainly possible for the calls to palloc() or
fmgr_info() to fail for legitimate reasons during normal usage (e.g. OOM
in the case of palloc(), or a multitude of reasons for fmgr_info()).

I'm not sure if there's a clean way to fix the problem. One solution
would be to enclose code that does allocations in TopMemoryContext in a
PG_TRY block. We could then pfree() the local variable in a PG_CATCH
block (provided that the "global = local_var" assignment hasn't occurred
yet). Unfortunately, that's pretty ugly, and requires adding hackery to
every site where this coding pattern occurs.

Thoughts?

-Neil

P.S. It occurs to me that the mbutils code is actually busted anyway, as
it assumes that a pfree'ing the FmgrInfo is sufficient to release all
the resources allocated by fmgr_info(). However, this is not the case:
the CurrentMemoryContext when fmgr_info() is called is used to allocate
additional things. mbutils ought to invoke fmgr_info() in its own
long-lived memory context -- but that is unrelated to the problem
described above...

[1] This codepath is only followed if the server_encoding is not
compatiable with the chosen client_encoding -- in my case
server_encoding = 'en_CA.utf8' reproduces the problem.)

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2006-01-11 07:58:32 Re: leaks in TopMemoryContext?
Previous Message Tom Lane 2006-01-11 05:08:45 Re: Is Optimizer smart enough?