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

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