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

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.
>>
>> $ psql
>> =# ALTER SYSTEM SET shared_buffers = '512MB';
>> ALTER SYSTEM
>> =# \q
>> $ pg_ctl -D data reload
>> server signaled
>> 2013-12-22 18:24:13 JST LOG: received SIGHUP, reloading configuration files
>> 2013-12-22 18:24:13 JST LOG: parameter "shared_buffers" cannot be
>> changed without restarting the server
>> 2013-12-22 18:24:13 JST LOG: configuration file "X??" contains
>> errors; unaffected changes were applied
>>
>> The configuration file name in the last log message is broken. This problem was
>> introduced by the ALTER SYSTEM SET patch.
>>
>>> 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.
>
> Note - In my m/c, I could not reproduce the issue. I think this is not
> surprising because we
> are not setting freed memory to NULL, so it can display anything
> (sometimes right value as well)

Couldn't we also handle this by postponing FreeConfigVariables until
after the if (error) block?

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?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company