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