On Friday, July 12, 2013 12:02 AM Fujii Masao wrote:
On Fri, Jul 5, 2013 at 2:19 PM, Amit Kapila <[email protected]> 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
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
