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.kapila@huawei.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