Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-06-18 14:11 keltezéssel, Amit Kapila írta:
> On Tuesday, June 18, 2013 3:26 PM Boszormenyi Zoltan wrote:
>> Hi,
>> review below.
> Thanks for the review.
>
>
>>>>>> There are 2 options to proceed for this patch for 9.4
>>>>>> 1. Upload the SET PERSISTENT syntax patch for coming CF by fixing
>>>>>> existing
>>>>>> review comments
>>>>>> 2. Implement new syntax ALTER SYSTEM as proposed in below mail
>>>>>> Could you suggest me what could be best way to proceed for this
>>>>>> patch?
>>>>> I'm still in favor of some syntax involving ALTER, because it's still
>>>>> true that this behaves more like the existing GUC-setting commands
>>>>> that use ALTER (which change configuration for future sessions)
>>>>> rather
>>>>> the ones that use SET (which change the current settings for some
>>>>> period of time).
>
>>>> I will change the patch as per below syntax if there are no objections:
>>>> ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'};
>>> Modified patch contains:
>>> 1. Syntax implemented is
>>> ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value' |
>>> DEFAULT};
>>> If user specifies DEFAULT, it will remove entry from auto conf file.
>>> 2. File name to store settings set by ALTER SYSTEM command is still
>>> persistent.auto.conf
>>> 3. Added a new page for Alter System command in docs. In compatability
>>> section, I had mentioned that
>>> this statement is an PostgresQL extension. I had tried to search in
>>> SQL-92 spec but couldn't find such command.
>>> 4. During test of this patch, I had observed one problem for PGC_BACKEND
>>> parameters like log_connections.
> >> If I change these parameters directly in postgresql.conf and then do
>>> pg_reload_conf() and reconnect, it will
>>> still show the old value. This is observed only on WINDOWS. I will
>>> investigate this problem and update you.
>>> Due to this problem, I had removed few test cases.
>> I can't reproduce this error under Linux (Fedora 19/x86_64).
>> The bug might be somewhere else and not caused by your patch
>> if manually adding/removing "log_connections = on" from postgresql.conf
>> exhibits the same behaviour under Windows. (If I read it correctly,
>> you tested it exactly this way.) Does the current GIT version behave
>> the same way?
> Yes, the current Git has problem which I had reported in below mail:
>
> http://www.postgresql.org/message-id/009801ce68f7$3a746340$af5d29c0$@kapila@
> huawei.com
>
> This problem is not related to this patch, it occurs only on WINDOWS or
> under EXEC_BACKEND flag.
> I think we can discuss this problem in above mail thread.
>
>
>
>> * Have all the bases been covered?
>> According to the above note under Windows, not yet.
>> The name "persistent.auto.conf" is not yet set in stone.
>> postgresql.auto.conf might be better.
> I think, the decision of name, we can leave to committer with below
> possibilities,
> as it is very difficult to build consensus on any particular name.
>
> Auto.conf
> System.auto.conf
> Postgresql.auto.conf
> Persistent.auto.conf
>
>> Apply the patch, compile it and test:
>
>> * Does it follow the project coding guidelines?
>> Mostly, yes. Some nits, though. At some places, you do:
>> - ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, &head, &tail);
>> + ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, &head,
> &tail,NULL);
>
> I think you mean ParseConfigFp()?

Sorry, yes.

>
> without a space between the last comma and the NULL keyword.
>
>> Also, in the comment above ParseConfigFile() there are unnecessary
>> white space changes and spaces at the end of the line.
> Do you mean to say whitespaces in below text?
>
> ! * and absolute-ifying the path name if necessary.
>
> ! *
> ! * While parsing, it records if it has parsed persistent.auto.conf file.
> ! * This information can be used by the callers to ensure if the parameters
> ! * set by ALTER SYSTEM SET command will be effective.
> */

Yes.

Anyway, both would be fixed by a pgindent run. (I think.)

>
> With Regards,
> Amit Kapila.
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/