On Tue, Jan 7, 2014 at 12:52 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jan 6, 2014 at 9:48 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>> Couldn't we also handle this by postponing FreeConfigVariables until
>>> after the if (error) block?
>>
>> Wouldn't doing that way can lead to bigger memory leak, if error level
>> is ERROR. Though in current fix also it can leak memory but it will be
>> just for ErrorConfFile_save. I think some similar case can happen for
>> 'pre_value' in code currently as well, that's why I have fixed it in a
>> similar way in patch.
>
> I was assuming that error-recovery would reset the containing memory
> context, but I'm not sure what memory context we're executing in at
> this point.
This function is called from multiple places and based on when it would
get called the memory context varies. During Startup, it gets called in
Postmaster context and if some backend runs pg_reload_conf(), then
it will get called from other background processes (WAL Writer,
Checpointer, etc..) in their respective contexts (for WAL Writer, the
context will be WAL Writer, ..).
In current code, the only time it can go to error path with elevel as
ERROR is during Postmaster startup
(context == PGC_POSTMASTER), at which it will anyway upgrade
ERROR to FATAL, so it should not be a problem to move
function FreeConfigVariables() after error block check. However
in future, if someone added any more ERROR (the chances of which
seems to be quite less), it can cause leak, may be thats why original
code has been written that way.
If you think it's better to fix by moving FreeConfigVariables() after error
block check, then I can update the patch by doing so and incorporate other
change (directly use PG_AUTOCONF_FILENAME) suggested by you
as well?
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
