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

Hi,

review below.

2013-06-13 14:35 keltezéssel, Amit Kapila írta:
> On Friday, June 07, 2013 9:45 AM Amit Kapila wrote:
>> On Thursday, June 06, 2013 10:22 PM Robert Haas wrote:
>>> On Wed, Jun 5, 2013 at 7:24 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
>>> wrote:
>>>> On Monday, May 27, 2013 4:17 PM Amit Kapila wrote:
>>>>> On Wednesday, April 03, 2013 11:55 AM Amit Kapila wote:
>>>>>> On Tuesday, April 02, 2013 9:49 PM Peter Eisentraut wrote:
>>>> 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?

>
> Kindly let me know your suggestions.
>
> With Regards,
> Amit Kapila.

* Is the patch in a patch format which has context?

Yes.

* Does it apply cleanly to the current git master?

Yes.

* Does it include reasonable tests, necessary doc patches, etc?

Yes.

Read what the patch is supposed to do, and consider:

* Does the patch actually implement that?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

No such spec, follows the agreed behaviour.

* Does it include pg_dump support (if applicable)?

N/A

* Are there dangers?

No.

* 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.

Apply the patch, compile it and test:

* Does the feature work as advertised?

Yes, with the noted exception above for log_connections.

* Are there corner cases the author has failed to consider?

I can't see any. It is through

* Are there any assertion failures or crashes?

No.

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

N/A

* Does it slow down other things?

No.

* Does it follow the project coding guidelines
<http://developer.postgresql.org/pgdocs/postgres/source.html>?

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);

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.

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should. It doesn't use any platform-dependent code.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

Yes.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

Best regards,
Zoltán Böszörményi

--
----------------------------------
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/