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

On Tue, Dec 24, 2013 at 10:08 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Dec 24, 2013 at 2:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Sun, Dec 22, 2013 at 8:32 PM, 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)
>>
>> 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?

I have run into this problem on many occasions because my benchmark
scripts usually append a hunk of stuff to postgresql.conf, and any
parameters that were already set generate this warning every time you
reload, because postgresql.conf now mentions that parameter twice.
I'm not quite sure what other problem you want to fix it together
with, and I'm not sure what the right fix is either, but +1 for coming
up with some solution that's better than what we have now.

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