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

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(dot)kapila(at)huawei(dot)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().

With Regards,
Amit Kapila.