Re: [BUGS] 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