Re: ALTER SYSTEM SET command to change postgresql.conf parameters

On Mon, Jan 6, 2014 at 7:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sun, Dec 22, 2013 at 10:02 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> I found the bug of ALTER SYSTEM SET patch. The procedure to reproduce
>>> it is here.
>>>
>>>> FreeConfigVariables(head);
>>>> <snip>
>>>> else if (apply)
>>>> ereport(elevel,
>>>> (errcode(ERRCODE_CONFIG_FILE_ERROR),
>>>> errmsg("configuration file \"%s\" contains errors; unaffected changes were applied",
>>>> ErrorConfFile)));
>>>
>>> The cause of the problem is that, in ProcessConfigFile(), the log
>>> message including
>>> the 'ErrorConfFile' is emitted after 'head' is free'd even though
>>> 'ErrorConfFile' points
>>> to one of entry of the 'head'.
>>
>> Your analysis is absolutely right.
>> The reason for this issue is that we want to display filename to which
>> erroneous parameter
>> belongs and in this case we are freeing the parameter info before actual error.
>> To fix, we need to save the filename value before freeing parameter list.
>>
>> Please find the attached patch to fix this issue.
>>
>>
>
> 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.

> Also, what's the point of this:
>
> + snprintf(ConfigAutoFileName,sizeof(ConfigAutoFileName),"%s",
> PG_AUTOCONF_FILENAME);
>
> Why copy PG_AUTOCONF_FILENAME into another buffer? Why not just pass
> PG_AUTOCONF_FILENAME directly to ParseConfigFile?

Initially, I think we were doing concat of folder and file name for
Autofile, that's why
the code was written that way, but currently it can be changed to way you are
suggesting.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com