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

On Mon, Jul 15, 2013 at 10:43 PM, Amit Kapila <amit.kapila@huawei.com> wrote:
> On Friday, July 12, 2013 6:46 PM Amit kapila wrote:
>> On Friday, July 12, 2013 12:02 AM Fujii Masao wrote:
>> 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 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> !=
>> <>
>
>     In gram.y semicolon (';') was missing, due to which it was not
> generating proper preproc.y
>
>> I generally use windows as dev environment, it hasn't shown these
>> warnings.
>> I shall check in linux and correct the same.
>
>   Fixed.
>>
>> > 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 tests in alter_system.sql are dependent on default values of
>> postgresql.conf, which is okay for make check
>> but not for installcheck. I shall remove that dependency from
>> testcases.
>
>   Removed all tests for which values were dependent on postgresql.conf
>   a. removed check of values before reload
>   b. removed parameters which can only set after server restart
>   c. removed check for values after setting to default
>
>>
>> > 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.
>>
>> The sleep is used to ensure the effects of pg_reload_conf() can be
>> visible.
>> To avoid increasing time of regression tests, we can do one of below:
>>
>> 1) decrease the time for sleep, but not sure how to safely decide how
>> much to sleep.
>> 2) combine the tests such that only 1 or 2 sleep calls should be used.
>>
>> what's your opinion?
>
> Modified to use 2 sleep calls.
>
>> > +               /* 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.
>>
>>   The original intention was to skip the autoconf temporary file which
>> is created during AlterSystemSetConfigFile()
>>   for crash safety. I will change to exclude temp autoconf file.
>
>    I had modified the check to exclude postgresql.auto.conf.temp file. I
> have used hardcoded ".temp" instead of macro,
> as I don't find other use of same in code.
>
>> > +       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().
>>
>>   Yes, it is better to use writefile(). I shall update the patch for
>> same.
>
>     Modified to use writefile().

Thanks for updating the patch!

In the patch, ALTER SYSTEM SET validates the postgresql.conf.
I think this is overkill. While ALTER SYSTEM SET is being executed,
a user might be modifying the postgresql.conf. That validation
breaks this use case.

+# This includes the default configuration directory included to support
+# ALTER SYSTEM statement.
+#
+# WARNING: User should not remove below include_dir or directory config,
+#          as both are required to make ALTER SYSTEM command work.
+#          Any configuration parameter values specified after this line
+#          will override the values set by ALTER SYSTEM statement.
+#include_dir = 'config'

Why do we need to expose this setting to a user? As the warning says,
if a user *should not* remove this setting, I think we should not expose
it from the beginning and should always treat the postgresql.auto.conf
as included configuration file even if that setting is not in postgresql.conf.

Regards,

--
Fujii Masao