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()?
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.
*/
With Regards,
Amit Kapila.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
