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

<div class="moz-cite-prefix">Hi,<br /><br /> review below.<br /><br /> 2013-06-13 14:35 keltezéssel, Amit Kapila
írta:<br/></div><blockquote cite="mid:009401ce6832$76653200$632f9600$@kapila@huawei.com" type="cite"><pre wrap="">On
Friday,June 07, 2013 9:45 AM Amit Kapila wrote:
 
</pre><blockquote type="cite"><pre wrap="">On Thursday, June 06, 2013 10:22 PM Robert Haas wrote:
</pre><blockquote type="cite"><pre wrap="">On Wed, Jun 5, 2013 at 7:24 AM, Amit Kapila <a class="moz-txt-link-rfc2396E"
href="mailto:amit.kapila@huawei.com"><amit.kapila@huawei.com></a>
wrote:
</pre><blockquote type="cite"><pre wrap="">On Monday, May 27, 2013 4:17 PM Amit Kapila wrote:
</pre><blockquote type="cite"><pre wrap="">On Wednesday, April 03, 2013 11:55 AM Amit Kapila wote:
</pre><blockquote type="cite"><pre wrap="">On Tuesday, April 02, 2013 9:49 PM Peter Eisentraut wrote:
</pre></blockquote><pre wrap="">
</pre></blockquote><pre wrap="">
There are 2 options to proceed for this patch for 9.4

1. Upload the SET PERSISTENT syntax patch for coming CF by fixing
</pre></blockquote><pre wrap="">existing
</pre><blockquote type="cite"><pre wrap="">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
</pre></blockquote><pre wrap="">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)
</pre></blockquote><pre wrap="">rather
</pre><blockquote type="cite"><pre wrap="">the ones that use SET (which change the current settings for some
period of time).
</pre></blockquote><pre wrap="">

I will change the patch as per below syntax if there are no objections:
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'};
</pre></blockquote><pre wrap="">
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.</pre></blockquote><br /> I
can'treproduce this error under Linux (Fedora 19/x86_64).<br /><br /> The bug might be somewhere else and not caused by
yourpatch<br /> if manually adding/removing "log_connections = on" from postgresql.conf<br /> exhibits the same
behaviourunder Windows. (If I read it correctly,<br /> you tested it exactly this way.) Does the current GIT version
behave<br/> the same way?<br /><br /><blockquote cite="mid:009401ce6832$76653200$632f9600$@kapila@huawei.com"
type="cite"><prewrap="">
 

Kindly let me know your suggestions. 

With Regards,
Amit Kapila.
</pre></blockquote><br /> * Is the patch in a patch format which has context?<br /><br /> Yes.<br /><br /> * Does it
applycleanly to the current git master?<br /><br /> Yes.<br /><br /> * Does it include reasonable tests, necessary doc
patches,etc? <br /><br /> Yes.<br /><br /><p>Read what the patch is supposed to do, and consider:<br /><p>* Does the
patchactually implement that?<br /><p>Yes.<br /> * Do we want that?<br /><br /> Yes.<br /><br /> * Do we already have
it?<br/><br /> No.<br /><br /> * Does it follow SQL spec, or the community-agreed behavior?<br /><br /> No such spec,
followsthe agreed behaviour.<br /><br /> * Does it include pg_dump support (if applicable)?<br /><br /> N/A<br /><br />
*Are there dangers?<br /><br /> No.<br /><br /> * Have all the bases been covered?<br /><br /> According to the above
noteunder Windows, not yet.<br /><br /> The name "persistent.auto.conf" is not yet set in stone.<br />
postgresql.auto.confmight be better.<br /><br /> Apply the patch, compile it and test:<br /><br /> * Does the feature
workas advertised?<br /><br /> Yes, with the noted exception above for log_connections.<br /><br /> * Are there corner
casesthe author has failed to consider?<br /><br /> I can't see any. It is through <br /><br /> * Are there any
assertionfailures or crashes? <br /><br /> No.<br /><br /> * Does the patch slow down simple tests?<br /><br /> No.<br
/><br/> * If it claims to improve performance, does it?<br /><br /> N/A<br /><br /> * Does it slow down other things?
<br/><br /> No.<br /><br /> * Does it follow the project <a class="external     text"
href="http://developer.postgresql.org/pgdocs/postgres/source.html"rel="nofollow">coding guidelines</a>?<br /><br />
Mostly,yes. Some nits, though. At some places, you do:<br /><br /> - ParseConfigFile(ConfigFileName, NULL, true, 0,
elevel,&head, &tail);<br /> + ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, &head,
&tail,NULL);<br/><br /> without a space between the last comma and the NULL keyword.<br /><br /> Also, in the
commentabove ParseConfigFile() there are unnecessary<br /> white space changes and spaces at the end of the line.<br
/><br/> * Are there portability issues?<br /><br /> No.<br /><br /> * Will it work on Windows/BSD etc?<br /><br /> It
should.It doesn't use any platform-dependent code.<br /><br /> * Are the comments sufficient and accurate?<br /><br />
Yes.<br/><br /> * Does it do what it says, correctly?<br /><br /> Yes.<br /><br /> * Does it produce compiler
warnings?<br/><br /> No.<br /><br /> * Can you make it crash?<br /><br /> No.<br /><br /> Best regards,<br /> Zoltán
Böszörményi<br/><br /><pre class="moz-signature" cols="90">-- 
 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: <a class="moz-txt-link-freetext" href="http://www.postgresql-support.de">http://www.postgresql-support.de</a>
<aclass="moz-txt-link-freetext" href="http://www.postgresql.at/">http://www.postgresql.at/</a>
 
</pre>