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

On 16 November 2013 09:43 Amit Kapila wrote:
> On Fri, Nov 15, 2013 at 10:18 PM, Peter Eisentraut <peter_e(at)gmx(dot)net>
> wrote:
> > On 11/14/13, 2:50 AM, Amit Kapila wrote:
> >> Find the rebased version attached with this mail.
> >
> > Doesn't build:
> >
> > openjade -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D .
> -c /usr/share/sgml/docbook/stylesheet/dsssl/modular/catalog -d
> stylesheet.dsl -t sgml -i output-html -V html-index postgres.sgml
> > openjade:reference.sgml:61:3:E: cannot find "alter_system.sgml";
> tried "ref/alter_system.sgml", "./alter_system.sgml",
> "./alter_system.sgml", "/usr/local/share/sgml/alter_system.sgml",
> "/usr/share/sgml/alter_system.sgml"
> > openjade:config.sgml:164:27:X: reference to non-existent ID "SQL-
> ALTERSYSTEM"
> > make[3]: *** [HTML.index] Error 1
> > make[3]: *** Deleting file `HTML.index'
> > osx -D. -x lower -i include-xslt-index postgres.sgml >postgres.xmltmp
> > osx:reference.sgml:61:3:E: cannot find "alter_system.sgml"; tried
> "ref/alter_system.sgml", "./alter_system.sgml",
> "/usr/local/share/sgml/alter_system.sgml",
> "/usr/share/sgml/alter_system.sgml"
> > osx:config.sgml:164:27:X: reference to non-existent ID "SQL-
> ALTERSYSTEM"
> > make[3]: *** [postgres.xml] Error 1
> >
> > New file missing in patch?
>
> Oops, missed the new sgml file in patch, updated patch to include it.
> Many thanks for checking it.

1. Patch applied properly
2. No warnings in the compilation
3. Regress test passed.
4. Basic tests are passed.

Please find review comments:

+ * ALTER SYSTEM SET
+ *
+ * Command to edit postgresql.conf
+ *****************************************************************************/

I feel it should be "Command to change the configuration parameter"
because this command is not edits the postgresql.conf file.

ereport(ERROR,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("configuration file \"%s\" contains errors",
- ConfigFileName)));
+ ErrorConfFile)));

The ErrorConfFile prints "postgresql.auto.conf" only if there is any parsing problem
with postgresql.auto.conf otherwise it always print "postgresql.conf" because of any other error.

+ * A stale temporary file may be left behind in case we crash.
+ * Such files are removed on the next server restart.

The above comment is wrong, the stale temporary file will be used
in the next ALTER SYSTEM command. I didn't find any code where it gets
deleted on the next server restart.

if any postmaster setting which are set by the alter system command which
leads to failure of server start, what is the solution to user to proceed
further to start the server. As it is mentioned that the auto.conf file
shouldn't be edited manually.

Regards,
Hari babu.