On Sun, Dec 22, 2013 at 10:02 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao <masao.fu...@gmail.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';
>> =# \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",

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to