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

On Fri, Jul 5, 2013 at 2:19 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Wednesday, July 03, 2013 2:58 AM Alvaro Herrera wrote:
>> Amit Kapila escribió:
>>
>> > I have changed the file name to postgresql.auto.conf and I have
>> changed the
>> > name of SetPersistentLock to AutoFileLock.
>> >
>> > Zoltan,
>> >
>> > Could you please once check the attached Patch if you have time, as
>> now all
>> > the things are resolved except for small doubt in syntax
>> extensibility
>> > which can be changed based on final decision.
>>
>> There are a bunch of whitespace problems, as you would notice with "git
>> diff --check" or "git show --color". Could you please clean that up?
>
> Fixed all whitespaces.
>
>> Also, some of the indentation in various places is not right; perhaps
>> you could fix that by running pgindent over the source files you
>> modified (this is easily visible by eyeballing the git diff output;
>> stuff is misaligned in pretty obvious ways.)
>
> Fixed, by running pgindent
>
>
>
>> Random minor other comments:
>>
>> + use of <xref linkend="SQL-ALTER SYSTEM">
>>
>> this SGML link cannot possibly work. Please run "make check" in the
>> doc/src/sgml directory.
>
> Fixed, make check passes now.
>
>> Examples in alter_system.sgml have a typo "SYTEM". Same file has "or
>> or".
>
> Fixed.
>
>> Also in that file,
>> The name of an configuration parameter that exist in
>> <filename>postgresql.conf</filename>.
>> the parameter needn't exist in that file to be settable, right?
>
> Changed to below text:
> Name of a settable run-time parameter. Available parameters are documented
> in <xref linkend="runtime-config">.
>
>
>> <refnamediv>
>> <refname>ALTER SYSTEM</refname>
>> <refpurpose>change a server configuration parameter</refpurpose>
>> </refnamediv>
>>
>> Perhaps "permanently change .."?
>
> Not changed.
>
>>
>> Also, some parameters require a restart, not a reload; maybe we should
>> direct the user somewhere else to learn how to freshen up the
>> configuration after calling the command.
>>
>> + /* skip auto conf temporary file */
>> + if (strncmp(de->d_name,
>> + PG_AUTOCONF_FILENAME ".",
>> + sizeof(PG_AUTOCONF_FILENAME)) == 0)
>> + continue;
>>
>> What's the dot doing there?
>
> Fixed, removed dot.
>>
>>
>> + if ((stat(AutoConfFileName, &st) == -1))
>> + ereport(elevel,
>> + (errmsg("configuration parameters changed with ALTER
>> SYSTEM command before restart of server "
>> + "will not be effective as \"%s\" file is not
>> accessible.", PG_AUTOCONF_FILENAME)));
>> + else
>> + ereport(elevel,
>> + (errmsg("configuration parameters changed with ALTER
>> SYSTEM command before restart of server "
>> + "will not be effective as default include
>> directive for \"%s\" folder is not present in postgresql.conf",
>> PG_AUTOCONF_DIR)));
>>
>> These messages should be split. Perhaps have the "will not be
>> effective" in the errmsg() and the reason as part of errdetail()?
>
> Okay, changed as per suggestion.
>
>> Also,
>> I'm not really sure about doing another stat() on the file after
>> parsing
>> failed; I think the parse routine should fill some failure context
>> information, instead of having this code trying to reproduce the
>> failure
>> to know what to report. Maybe something like the SlruErrorCause enum,
>> not sure.
>>
>> Similarly,
>>
>> + /*
>> + * record if the file currently being parsed is
>> postgresql.auto.conf,
>> + * so that it can be later used to give warning if it doesn't
>> parse
>> + * it.
>> + */
>> + snprintf(Filename,sizeof(Filename),"%s/%s", PG_AUTOCONF_DIR,
>> PG_AUTOCONF_FILENAME);
>> + ConfigAutoFileName = AbsoluteConfigLocation(Filename,
>> ConfigFileName);
>> + if (depth == 1 && strcmp(ConfigAutoFileName, config_file) == 0)
>> + *is_config_dir_parsed = true;
>>
>> this seems very odd. The whole "is_config_dir_parsed" mechanism smells
>> strange to me, really. I think the caller should be in charge of
>> keeping track of this, but I'm not sure. ParseConfigFp() would have no
>> way of knowing, and in one place we're calling that routine precisely
>> to
>> parse the auto file.
>
> Changed by adding new enum AutoConfErrorCause. Now is_config_dir_parsed is
> removed from code.
> Kindly let me know if this way is better than previous?
>
> Apart from above I have fixed one issue in function
> AlterSystemSetConfigFile(), called FreeConfigVariables().

I got the following compile warnings.

guc.c:5187: warning: no previous prototype for 'validate_conf_option'
preproc.y:7746.2-31: warning: type clash on default action: <str> != <>

The make installcheck failed when I ran it against the server with
wal_level = hot_standby. The regression.diff is

------------------------------------
*** 27,35 ****
(1 row)

show wal_level;
! wal_level
! -----------
! minimal
(1 row)

show authentication_timeout;
--- 27,35 ----
(1 row)

show wal_level;
! wal_level
! -------------
! hot_standby
(1 row)

show authentication_timeout;
------------------------------------

The regression test of ALTER SYSTEM calls pg_sleep(1) six times.
Those who dislike the regression test patches which were proposed
in this CF might dislike this repeat of pg_sleep(1) because it would
increase the time of regression test.

+ /* skip auto conf temporary file */
+ if (strncmp(de->d_name,
+ PG_AUTOCONF_FILENAME,
+ sizeof(PG_AUTOCONF_FILENAME)) == 0)
+ continue;

Why do we need to exclude the auto conf file from the backup?
I think that it should be included in the backup as well as normal
postgresql.conf.

+ autofile = fopen(path, PG_BINARY_W);
+ if (autofile == NULL)
+ {
+ fprintf(stderr, _("%s: could not open file \"%s\" for writing: %s\n"),
+ progname, path, strerror(errno));
+ exit_nicely();
+ }
+
+ if (fputs("# Do not edit this file manually! It is overwritten by
the ALTER SYSTEM command \n",
+ autofile) < 0)
+ {
+ fprintf(stderr, _("%s: could not write file \"%s\": %s\n"),
+ progname, path, strerror(errno));
+ exit_nicely();
+ }
+
+ if (fclose(autofile))
+ {
+ fprintf(stderr, _("%s: could not close file \"%s\": %s\n"),
+ progname, path, strerror(errno));
+ exit_nicely();
+ }

You can simplify the code by using writefile().

Regards,

--
Fujii Masao