BUG #7642: Unchecked “stats_temp_directory” causes server hang to requests

OSH, why the format becomes so ugly?!!

Here's the original mail.

------------------------

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 SetConfigOption(const char *name, const char *value,
                                GucContext context, GucSource source);

+extern void checkConfPath(void);
+
extern void DefineCustomBoolVariable(
const char *name,
const char *short_desc,

diff --git
a/../postgresql_tmp/postgresql-9.2.1/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 3a4cef3..883409b 100644
--- a/../postgresql_tmp/postgresql-9.2.1/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -756,6 +756,9 @@ PostmasterMain(int argc, char *argv[])
        /* And switch working directory 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, <tixu@cs.ucsd.edu> wrote:

The following bug has been logged on the website:

Bug reference: 7642
Logged by: Tianyin Xu
Email address: tixu@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 SetConfigOption(const char *name, const char *value,

GucContext context, GucSource source);

+extern void checkConfPath(void);

+

extern void DefineCustomBoolVariable(

const char *name,

const char *short_desc,

diff --git
a/../postgresql_tmp/postgresql-9.2.1/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c

index 3a4cef3..883409b 100644

---
a/../postgresql_tmp/postgresql-9.2.1/src/backend/postmaster/postmaster.c

+++ b/src/backend/postmaster/postmaster.c

@@ -756,6 +756,9 @@ PostmasterMain(int argc, char *argv[])

/* And switch working directory 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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

--
Tianyin XU,
http://cseweb.ucsd.edu/~tixu/

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

regards, tom lane

Thanks for the reply, Tom.

On Wed, Nov 7, 2012 at 10:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

tixu@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/