On Mon, Jan 6, 2014 at 11:37 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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?
Yeah, let's do it that way.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company