On Thu, Jan 9, 2014 at 11:06 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Jan 9, 2014 at 12:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> 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.
>>>
>>>
>>> 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.
>
> Okay, done. Attached patch fixes both the display of wrong file name and
> usage of PG_AUTOCONF_FILENAME.
Committed with a comment change.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company