Re: [BUGS] BUG #7624: Misleading Log Message & Inconsistent Configuration Design
Hi, Craig, Thanks a lot for your response. Sorry to make the bug report so long... I was a little pissed off by myself for my stupidness after spending around 2 hours on this issue. Yes, your message is definitely better and explicit by pointing out CWD. The only concern to me is whether or not CWD is a common abbreviation, especially for non-native speakers. Searching "CWD" on google, I didn't find it refers to Current Working Directory in the first 3 returned pages. To me, printing an explicit log message is the easiest way to solve the problem. But the configuration file design is still not consistent (maybe I'm too picky). In the following settings, we have to tell users that the first 3 works and the last 1 does not work because of chdir blah blah blah. data_directory = 'local/pgsql/data/' hba_file = 'local/pgsql/data/pg_hba.conf' ident_file = 'local/pgsql/data/pg_ident.conf' unix_socket_directory = 'local/pgsql/data/' Thanks, Tianyin On Mon, Oct 29, 2012 at 7:46 PM, Craig Ringer wrote: > On 10/28/2012 03:35 PM, t...@cs.ucsd.edu wrote: > > I really suggest to make the configuration file consistent in this case. > But > > I understand it might not be easy. But at least I think we should print a > > better log message which pinpoints to the absolute path like > > > > FATAL: 58P01: could not create lock file > > > "/home/tianyin/postgresql-9.2.1/local/pgsql/data/local/pgsql/data/.s.PGSQL.5432.lock": > > No such file or directory > > Essentially, you want PostgreSQL to print absolute paths in error > messages, instead of paths relative to the server datadir? > > I'd prefer to do it slightly differently, but I like the general idea. > I'd instead want to write: > > FATAL: 58P01: could not create lock file > "local/pgsql/data/.s.PGSQL.5432.lock" > (cwd="/home/tianyin/postgresql-9.2.1/local/pgsql/data/"): No such file > or directory > > By spelling out the CWD explicitly there's no impression given that the > user ever actually specified the whole path in the configuration > anywhere, so it's clearer how the path came to be. It also shows which > part of the path is known-good (since it's the CWD) and which could be > at issue. > > -- > Craig Ringer > -- Tianyin XU, http://cseweb.ucsd.edu/~tixu/
[BUGS] Re: [BUGS] BUG #7642: Unchecked “stats_temp_directory” causes server hang to requests
y into it */
ChangeToDataDir();
+/* Check whether user input paths make sense to avoid later
problems */
+checkConfPath();
+
/*
* Check for invalid combinations of GUC settings.
*/
Thanks a lot!
Tianyin
On Wed, Nov 7, 2012 at 10:33 AM, wrote:
> The following bug has been logged on the website:
>
> Bug reference: 7642
> Logged by: Tianyin Xu
> Email address: t...@cs.ucsd.edu
> PostgreSQL version: 9.2.1
> Operating system: Linux
> Description:
>
> Hi, Postgresql,
>
> I set the stats_temp_directory='' in my postgresql.conf file, with the
> expectation that the directory would be under the data directory. However,
> the server convert the file to “/pgstat.tmp” by which it has no permission.
> The bad thing is that the error messages start flying all over my screen,
> and the server hangs for requests (because of I/O overhead?):
>
>
> LOG: could not open temporary statistics file "/pgstat.tmp": Permission
> denied
>
> LOG: could not open temporary statistics file "/pgstat.tmp": Permission
> denied
>
> LOG: could not open temporary statistics file "/pgstat.tmp": Permission
> denied
>
> (another 31 lines)
>
> LOG: could not open temporary statistics file "/pgstat.tmp": Permission
> denied
>
> WARNING: pgstat wait timeout
>
>
> LOG: could not open temporary statistics file "/pgstat.tmp": Permission
> denied
>
> LOG: could not open temporary statistics file "/pgstat.tmp": Permission
> denied
>
> LOG: could not open temporary statistics file "/pgstat.tmp": Permission
> denied
>
> (another 31 lines)
>
> LOG: could not open temporary statistics file "/pgstat.tmp": Permission
> denied
>
> WARNING: pgstat wait timeout
>
> …...
>
> …...
>
> …...
>
>
> I found pg has several unchecked user configurations which might cause
> problems at run-time. I suggest to check it like what has been done for
> “data_directory” to avoid latter problems. Here is the patches. I propose
> to
> add a uniform checker somewhere to check the validity of all the user input
> directory/file path to avoid these problems in a systematic way.
>
>
> Here is my patches. Basically I want to make a checking function for file
> paths, in order to detect all the misconfigured paths before running the PG
> server. If the user input is wrong, pinpoint the directives and the wrong
> values.
>
>
> I hope it makes sense.
>
>
> diff --git
> a/../postgresql_tmp/postgresql-9.2.1/src/backend/utils/misc/guc.c
> b/src/backend/utils/misc/guc.c
>
> index 7f54d45..ac315e7 100644
>
> --- a/../postgresql_tmp/postgresql-9.2.1/src/backend/utils/misc/guc.c
>
> +++ b/src/backend/utils/misc/guc.c
>
> @@ -4201,6 +4201,48 @@ SelectConfigFiles(const char *userDoption, const
> char
> *progname)
>
> return true;
>
> }
>
>
> +void
>
> +checkPath(const char* path, const char* directive, bool isDir)
>
> +{
>
> + char *abs_tmpdir;
>
> + abs_tmpdir = make_absolute_path(path);
>
> +
>
> + struct stat stat_buf;
>
> +
>
> + Assert(abs_tmpdir);
>
> + if (stat(abs_tmpdir, &stat_buf) != 0) {
>
> + if (errno == ENOENT)
>
> + ereport(FATAL, (errcode_for_file_access(),
>
> + errmsg("%s \"%s\" does not exist",
>
> + directive,
>
> + abs_tmpdir)));
>
> + else
>
> + ereport(FATAL,
>
> + (errcode_for_file_access(),
>
> + errmsg("could not read permissions of %s \"%s\": %m",
>
> + directive,
>
> + abs_tmpdir)));
>
> +
>
> + }
>
> +
>
> + if(isDir) {
>
> + if (!S_ISDIR(stat_buf.st_mode))
>
> + ereport(FATAL,
>
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>
> + errmsg("specified %s \"%s\" is not a directory",
>
> + directive,
>
> + abs_tmpdir)));
>
> + }
>
> +}
>
> +
>
> +/* check whether user input are misconfigured */
>
> +void
>
> +checkConfPath(void)
>
> +{
>
> + checkPath(pgstat_temp_directory, "pgstat_temp_directory", true);
>
> + checkPath(Log_directory, "log_directory", true);
>
> + checkPath(UnixSocketDir, "unix_socket_directory", true);
>
> +}
>
>
>
> diff --git a/../postgresql_tmp/postgresql-9.2.1/src/include/utils/guc.h
> b/src/include/utils/guc.h
>
> index 6810387..1272207 100644
>
> --- a/../postgresql_tmp/postgresql-9.2.1/src/include/utils/guc.h
>
> +++ b/src/include/utils/guc.h
>
> @@ -231,6 +231,8 @@ extern int tcp_keepalives_count;
>
> extern void SetConfigOptio
[BUGS] Re: [BUGS] BUG #7642: Unchecked “stats_temp_directory” causes server hang to requests
Thanks for the reply, Tom. On Wed, Nov 7, 2012 at 10:53 AM, Tom Lane wrote: > t...@cs.ucsd.edu writes: > > I set the stats_temp_directory='' in my postgresql.conf file, with the > > expectation that the directory would be under the data directory. > > Why would you think that's a good idea? It's already going to be under > the data directory. The only reason to change this setting is if you're > going to point to a ramdisk somewhere, which is going to need an > absolute path. > > I just changed my previous setting to the empty one. I thought it would be under the data directory also. I got your point, I was stupid (I should comment this one if I don't need it). What I notice is that any wrong setting might cause latter problems. Since many of us redirect the log messages into files, sometimes we do not notice the problem immediately (at least for me). Maybe I'm too careless. For this one, when I noticed the problem, the log file was filled up with the same error message, and many requests stuck there... > > I found pg has several unchecked user configurations which might cause > > problems at run-time. I suggest to check it like what has been done for > > “data_directory” to avoid latter problems. Here is the patches. > > This patch seems to have got mangled by your mailer, but in any case I > don't really see the value. There are any number of ways to mess up > settings like these, and only some of them are detectable by tests > of this sort. Also, the proposed patch only checks at startup time; > if we're going to install training wheels, that doesn't seem sufficient. > > Yes, so I resend the previous mail. I agree with you that users might make thousands of different mistakes, and all the checkers are not complete. My personal opinion is to check as early as possible, instead of finding errors at runtime. I really like the idea of data directory checking and all the out-of-range checking of the numeric values. What I envisioned is similar checking for semantic values. I don't know whether it's possible (Sorry for my crappy patch). btw, what is training wheels? > regards, tom lane > -- Tianyin XU, http://cseweb.ucsd.edu/~tixu/
