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 7:39 PM Boszormenyi Zoltan wrote:
> 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.)

I think pgindent doesn't work on .l file, in anycase I had corrected it
manually.
Find the updated patch attached with this mail.

Kindly let me know if there is anything more needs to be done for this
patch.

With Regards,
Amit Kapila.