Re: ALTER SYSTEM SET command to change postgresql.conf parameters - Mailing list pgsql-hackers

On Tue, Dec 24, 2013 at 2:57 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sun, Dec 22, 2013 at 8:32 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao <masao.fujii@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';
>>> 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)
>
> If you find that the provided patch doesn't fix the problem or you don't
> find it appropriate due to some other reason, let me know the feedback.

When I read ProcessConfigFile() more carefully, I found another related
problem. The procedure to reproduce the problem is here.

--------------------
$ psql
=# ALTER SYSTEM SET shared_buffers = '256MB';
=# \q

$ echo "shared_buffers = '256MB'" >> $PGDATA/postgresql.conf
$ pg_ctl -D $PGDATA reload
2013-12-25 03:05:44 JST LOG:  parameter "shared_buffers" cannot be
changed without restarting the server
2013-12-25 03:05:44 JST LOG:  parameter "shared_buffers" cannot be
changed without restarting the server
2013-12-25 03:05:44 JST LOG:  configuration file
"/dav/alter-system/data/postgresql.auto.conf" contains errors;
unaffected changes were applied
--------------------

Both postgresql.conf and postgresql.auto.conf contain the setting of
the parameter that cannot be changed without restarting the server.
But only postgresql.auto.conf was logged, and which would be confusing,
I'm afraid. I think that this problem should be fixed together with the
problem that I reported before. Thought?

Regards,

-- 
Fujii Masao