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

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.