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

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> != <>

I generally use windows as dev environment, it hasn't shown these warnings.
I shall check in linux and correct the same.

> 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.


> 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?

> +               /* 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
crashsafety. I will change to exclude temp autoconf file. 

> +       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.

With Regards,
Amit Kapila