leaks in TopMemoryContext? - Mailing list 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.)