Why isn't stats_temp_directory automatically created?
Hi,
log_directory is automatically created if not present when starting
the database server. But, stats_temp_directory is not created. Why?
ISTM that current behavior is undesirable.
Is it worth making the patch which creates stats_temp_directory
if not present?
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Fujii Masao escreveu:
Is it worth making the patch which creates stats_temp_directory
if not present?
+1.
--
Euler Taveira de Oliveira
http://www.timbira.com/
Euler Taveira de Oliveira <euler@timbira.com> wrote:
Fujii Masao escreveu:
Is it worth making the patch which creates stats_temp_directory
if not present?+1.
+1, but AFAIK stats_temp_directory was designed to symlink to a RAM drive.
Even if stats_temp_directory exists as a symbolic link, the destination
directory might be lost in such a situation. If you try to make servers
more robust, you might also need to consider broken symlinks.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
Hi,
On Tue, Apr 14, 2009 at 10:26 PM, Euler Taveira de Oliveira
<euler@timbira.com> wrote:
Fujii Masao escreveu:
Is it worth making the patch which creates stats_temp_directory
if not present?+1.
Here is the patch.
This patch should be added to CommitFest-2009-First?,
or committed before 8.4 release? The patch is very small,
so I don't think that it'll block 8.4 release.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
create_stats_temp_dir_0415.patchtext/x-patch; charset=US-ASCII; name=create_stats_temp_dir_0415.patchDownload
Index: src/backend/postmaster/pgstat.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/postmaster/pgstat.c,v
retrieving revision 1.187
diff -c -r1.187 pgstat.c
*** src/backend/postmaster/pgstat.c 1 Jan 2009 17:23:46 -0000 1.187
--- src/backend/postmaster/pgstat.c 15 Apr 2009 06:08:04 -0000
***************
*** 111,116 ****
--- 111,117 ----
bool pgstat_track_counts = false;
int pgstat_track_functions = TRACK_FUNC_OFF;
int pgstat_track_activity_query_size = 1024;
+ char *pgstat_temp_directory;
/* ----------
* Built from GUC parameter
***************
*** 589,594 ****
--- 590,600 ----
return 0;
/*
+ * Create temporary statistics directory if not present; ignore errors
+ */
+ mkdir(pgstat_temp_directory, 0700);
+
+ /*
* Do nothing if too soon since last collector start. This is a safety
* valve to protect against continuous respawn attempts if the collector
* is dying immediately at launch. Note that since we will be re-called
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.502
diff -c -r1.502 guc.c
*** src/backend/utils/misc/guc.c 7 Apr 2009 23:27:34 -0000 1.502
--- src/backend/utils/misc/guc.c 15 Apr 2009 06:08:18 -0000
***************
*** 375,382 ****
char *IdentFileName;
char *external_pid_file;
- char *pgstat_temp_directory;
-
int tcp_keepalives_idle;
int tcp_keepalives_interval;
int tcp_keepalives_count;
--- 375,380 ----
Index: src/include/pgstat.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/pgstat.h,v
retrieving revision 1.82
diff -c -r1.82 pgstat.h
*** src/include/pgstat.h 4 Jan 2009 22:19:59 -0000 1.82
--- src/include/pgstat.h 15 Apr 2009 06:08:22 -0000
***************
*** 593,598 ****
--- 593,599 ----
extern bool pgstat_track_counts;
extern int pgstat_track_functions;
extern PGDLLIMPORT int pgstat_track_activity_query_size;
+ extern PGDLLIMPORT char *pgstat_temp_directory;
extern char *pgstat_stat_tmpname;
extern char *pgstat_stat_filename;
Fujii Masao wrote:
Hi,
On Tue, Apr 14, 2009 at 10:26 PM, Euler Taveira de Oliveira
<euler@timbira.com> wrote:Fujii Masao escreveu:
Is it worth making the patch which creates stats_temp_directory
if not present?+1.
Here is the patch.
This patch should be added to CommitFest-2009-First?,
or committed before 8.4 release? The patch is very small,
so I don't think that it'll block 8.4 release.
I think the fix should go into 8.4 - this is a fix for a new feature.
However, a couple of comments:
This does not take into account the effect of symlinks as mentioned by
Itakagi Takahiro. I haven't looked at the details, but I don't think it
would be that much more work to deal with it - and as he mentions, this
is a very common usecase.
Also, wouldn't it be better to isolate this to the first time when we
try to create the file - then we don't have to export the symbol?
//Magnus
Hi,
On Wed, Apr 15, 2009 at 5:37 PM, Magnus Hagander <magnus@hagander.net> wrote:
This does not take into account the effect of symlinks as mentioned by
Itakagi Takahiro. I haven't looked at the details, but I don't think it
would be that much more work to deal with it - and as he mentions, this
is a very common usecase.
Okey, I'll revise the patch; create also the directory which is
referenced by symlink if not present.
Also, wouldn't it be better to isolate this to the first time when we
try to create the file - then we don't have to export the symbol?
You mean having assign_pgstat_temp_directory() create the
directory instead of pgstat_start()? In this case, the directory is
created automatically not only at the beginning but also when
a configuration file is reloaded. This seems to be better behavior.
One problem of this is that some directories may be created
unexpectedly if stats_temp_directory is specified at multiple
locations. It's because assign_hook is called for each setting.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Fujii Masao wrote:
Hi,
Hi!
Sorry about the very late response - I've been out of the country and
generally busy.
On Wed, Apr 15, 2009 at 5:37 PM, Magnus Hagander <magnus@hagander.net> wrote:
This does not take into account the effect of symlinks as mentioned by
Itakagi Takahiro. I haven't looked at the details, but I don't think it
would be that much more work to deal with it - and as he mentions, this
is a very common usecase.Okey, I'll revise the patch; create also the directory which is
referenced by symlink if not present.
Great.
Also, wouldn't it be better to isolate this to the first time when we
try to create the file - then we don't have to export the symbol?You mean having assign_pgstat_temp_directory() create the
directory instead of pgstat_start()? In this case, the directory is
created automatically not only at the beginning but also when
a configuration file is reloaded. This seems to be better behavior.
No, I meant creating it when we open the file - in pgstat_write_statsfile().
//Magnus
Hi,
On Mon, Apr 20, 2009 at 1:29 AM, Magnus Hagander <magnus@hagander.net> wrote:
Sorry about the very late response - I've been out of the country and
generally busy.
Thanks for taking the time to comment!
On Wed, Apr 15, 2009 at 5:37 PM, Magnus Hagander <magnus@hagander.net> wrote:
This does not take into account the effect of symlinks as mentioned by
Itakagi Takahiro. I haven't looked at the details, but I don't think it
would be that much more work to deal with it - and as he mentions, this
is a very common usecase.Okey, I'll revise the patch; create also the directory which is
referenced by symlink if not present.Great.
Here is the revised patch; If stats_temp_directory indicates the symlink,
we pursue the chain of symlinks and create the referenced directory.
Also, wouldn't it be better to isolate this to the first time when we
try to create the file - then we don't have to export the symbol?You mean having assign_pgstat_temp_directory() create the
directory instead of pgstat_start()? In this case, the directory is
created automatically not only at the beginning but also when
a configuration file is reloaded. This seems to be better behavior.No, I meant creating it when we open the file - in pgstat_write_statsfile().
OK, I changed the patch so. Thanks.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
create_stats_temp_dir_0421.patchapplication/octet-stream; name=create_stats_temp_dir_0421.patchDownload
Index: src/backend/postmaster/pgstat.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/postmaster/pgstat.c,v
retrieving revision 1.187
diff -c -r1.187 pgstat.c
*** src/backend/postmaster/pgstat.c 1 Jan 2009 17:23:46 -0000 1.187
--- src/backend/postmaster/pgstat.c 21 Apr 2009 06:32:54 -0000
***************
*** 111,116 ****
--- 111,117 ----
bool pgstat_track_counts = false;
int pgstat_track_functions = TRACK_FUNC_OFF;
int pgstat_track_activity_query_size = 1024;
+ char *pgstat_temp_directory;
/* ----------
* Built from GUC parameter
***************
*** 2953,2958 ****
--- 2954,2964 ----
const char *statfile = permanent?PGSTAT_STAT_PERMANENT_FILENAME:pgstat_stat_filename;
/*
+ * Create temporary statistics directory if not present; ignore errors
+ */
+ CreateDir(pgstat_temp_directory, 0700);
+
+ /*
* Open the statistics temp file to write out the current values.
*/
fpout = AllocateFile(tmpfile, PG_BINARY_W);
Index: src/backend/storage/file/fd.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/file/fd.c,v
retrieving revision 1.148
diff -c -r1.148 fd.c
*** src/backend/storage/file/fd.c 4 Mar 2009 09:12:49 -0000 1.148
--- src/backend/storage/file/fd.c 21 Apr 2009 06:32:54 -0000
***************
*** 1875,1877 ****
--- 1875,2084 ----
FreeDir(temp_dir);
}
+
+ /* source stolen from FreeBSD /src/bin/mkdir/mkdir.c and adapted */
+
+ /*
+ * this tries to build all the elements of a path to a directory a la mkdir -p
+ * we assume the path is in canonical form, i.e. uses / as the separator
+ * we also assume it isn't null.
+ *
+ * note that on failure, the path arg has been modified to show the particular
+ * directory level we had problems with.
+ */
+ static int
+ mkdir_p(char *path, mode_t omode)
+ {
+ struct stat sb;
+ mode_t numask,
+ oumask;
+ int first,
+ last,
+ retval;
+ char *p;
+
+ p = path;
+ oumask = 0;
+ retval = 0;
+
+ #ifdef WIN32
+ /* skip network and drive specifiers for win32 */
+ if (strlen(p) >= 2)
+ {
+ if (p[0] == '/' && p[1] == '/')
+ {
+ /* network drive */
+ p = strstr(p + 2, "/");
+ if (p == NULL)
+ return 1;
+ }
+ else if (p[1] == ':' &&
+ ((p[0] >= 'a' && p[0] <= 'z') ||
+ (p[0] >= 'A' && p[0] <= 'Z')))
+ {
+ /* local drive */
+ p += 2;
+ }
+ }
+ #endif
+
+ if (p[0] == '/') /* Skip leading '/'. */
+ ++p;
+ for (first = 1, last = 0; !last; ++p)
+ {
+ if (p[0] == '\0')
+ last = 1;
+ else if (p[0] != '/')
+ continue;
+ *p = '\0';
+ if (!last && p[1] == '\0')
+ last = 1;
+ if (first)
+ {
+ /*
+ * POSIX 1003.2: For each dir operand that does not name an
+ * existing directory, effects equivalent to those caused by the
+ * following command shall occcur:
+ *
+ * mkdir -p -m $(umask -S),u+wx $(dirname dir) && mkdir [-m mode]
+ * dir
+ *
+ * We change the user's umask and then restore it, instead of
+ * doing chmod's.
+ */
+ oumask = umask(0);
+ numask = oumask & ~(S_IWUSR | S_IXUSR);
+ (void) umask(numask);
+ first = 0;
+ }
+ if (last)
+ (void) umask(oumask);
+
+ /* check for pre-existing directory; ok if it's a parent */
+ if (stat(path, &sb) == 0)
+ {
+ if (!S_ISDIR(sb.st_mode))
+ {
+ if (last)
+ errno = EEXIST;
+ else
+ errno = ENOTDIR;
+ retval = 1;
+ break;
+ }
+ }
+ else if (mkdir(path, last ? omode : S_IRWXU | S_IRWXG | S_IRWXO) < 0)
+ {
+ retval = 1;
+ break;
+ }
+ if (!last)
+ *p = '/';
+ }
+ if (!first && !last)
+ (void) umask(oumask);
+ return retval;
+ }
+
+ /* Create the specified directory if not present; ignore errors */
+ void
+ CreateDir(char *pathname, mode_t mode)
+ {
+ struct stat st;
+ char path[MAXPGPATH];
+ char orig_wd[MAXPGPATH];
+ char lnk_buf[MAXPGPATH];
+
+ /* Create the specified directory if not present */
+ if (lstat(pathname, &st) < 0)
+ {
+ mkdir_p(pathname, mode);
+ return;
+ }
+
+ /* Do nothing if the specified directory is present */
+ if (S_ISDIR(st.st_mode))
+ return;
+
+ /*
+ * If the specified 'pathname' indicates the symlink, we pursue
+ * the chain of it and create the referenced directory.
+ */
+ #ifdef HAVE_READLINK
+ StrNCpy(path, pathname, MAXPGPATH);
+
+ /*
+ * To resolve a symlink properly, we have to chdir into its directory and
+ * then chdir to where the symlink points; otherwise we may fail to
+ * resolve relative links correctly (consider cases involving mount
+ * points, for example). After following the final symlink, we use
+ * getcwd() to figure out where the heck we're at.
+ *
+ * One might think we could skip all this if path doesn't point to a
+ * symlink to start with, but that's wrong. We also want to get rid of
+ * any directory symlinks that are present in the given path. We expect
+ * getcwd() to give us an accurate, symlink-free path.
+ */
+ if (!getcwd(orig_wd, MAXPGPATH))
+ {
+ ereport(LOG,
+ (errmsg("could not identify current directory: %m")));
+ return;
+ }
+
+ for (;;)
+ {
+ char *lsep;
+ char *fname;
+ int rllen;
+
+ /*
+ * Create the referenced directory when the chain of symlinks
+ * can not be pursued any longer.
+ */
+ if (lstat(path, &st) < 0 || !S_ISLNK(st.st_mode))
+ {
+ mkdir_p(path, mode);
+ break;
+ }
+
+ lsep = last_dir_separator(path);
+ if (lsep)
+ {
+ *lsep = '\0';
+
+ if (chdir(path) == -1)
+ {
+ ereport(LOG,
+ (errmsg("could not change directory to \"%s\": %m",
+ path)));
+ break;
+ }
+
+ fname = lsep + 1;
+ }
+ else
+ fname = path;
+
+ rllen = readlink(fname, lnk_buf, MAXPGPATH);
+ if (rllen < 0 || rllen >= MAXPGPATH)
+ {
+ ereport(LOG,
+ (errmsg("could not read symbolic link \"%s\": %m",
+ fname)));
+ break;
+ }
+
+ lnk_buf[rllen] = '\0';
+ StrNCpy(path, lnk_buf, MAXPGPATH);
+ }
+
+ if (chdir(orig_wd) == -1)
+ {
+ ereport(LOG,
+ (errmsg("could not change directory to \"%s\": %m"),
+ orig_wd));
+ return;
+ }
+ #endif /* HAVE_READLINK */
+ }
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.502
diff -c -r1.502 guc.c
*** src/backend/utils/misc/guc.c 7 Apr 2009 23:27:34 -0000 1.502
--- src/backend/utils/misc/guc.c 21 Apr 2009 06:32:55 -0000
***************
*** 375,382 ****
char *IdentFileName;
char *external_pid_file;
- char *pgstat_temp_directory;
-
int tcp_keepalives_idle;
int tcp_keepalives_interval;
int tcp_keepalives_count;
--- 375,380 ----
Index: src/include/pgstat.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/pgstat.h,v
retrieving revision 1.82
diff -c -r1.82 pgstat.h
*** src/include/pgstat.h 4 Jan 2009 22:19:59 -0000 1.82
--- src/include/pgstat.h 21 Apr 2009 06:32:55 -0000
***************
*** 593,598 ****
--- 593,599 ----
extern bool pgstat_track_counts;
extern int pgstat_track_functions;
extern PGDLLIMPORT int pgstat_track_activity_query_size;
+ extern PGDLLIMPORT char *pgstat_temp_directory;
extern char *pgstat_stat_tmpname;
extern char *pgstat_stat_filename;
Index: src/include/storage/fd.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/storage/fd.h,v
retrieving revision 1.64
diff -c -r1.64 fd.h
*** src/include/storage/fd.h 12 Jan 2009 05:10:45 -0000 1.64
--- src/include/storage/fd.h 21 Apr 2009 06:32:55 -0000
***************
*** 98,103 ****
--- 98,105 ----
extern int pg_fsync_writethrough(int fd);
extern int pg_fdatasync(int fd);
+ extern void CreateDir(char *pathname, mode_t mode);
+
/* Filename components for OpenTemporaryFile */
#define PG_TEMP_FILES_DIR "pgsql_tmp"
#define PG_TEMP_FILE_PREFIX "pgsql_tmp"
Hi,
On Tue, Apr 21, 2009 at 4:33 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Here is the revised patch; If stats_temp_directory indicates the symlink,
we pursue the chain of symlinks and create the referenced directory.
BTW, this patch is useful also as the foundation for improving
creation of log_directory. Attached patch fixes the following
problem of log_directory by using that fundamental patch.
- log_directory is not created when a configuration file is reloaded
- creation of log_directory fails if the parent directory of it doesn't exist
- if log_directory indicates the symlink, it's not resolved
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
create_log_directory_0422.patchapplication/octet-stream; name=create_log_directory_0422.patchDownload
diff -crN orig/src/backend/postmaster/syslogger.c log_directory/src/backend/postmaster/syslogger.c
*** orig/src/backend/postmaster/syslogger.c Wed Apr 22 16:28:23 2009
--- log_directory/src/backend/postmaster/syslogger.c Wed Apr 22 16:00:47 2009
***************
*** 493,503 ****
#endif
/*
- * Create log directory if not present; ignore errors
- */
- mkdir(Log_directory, 0700);
-
- /*
* The initial logfile is created right in the postmaster, to verify that
* the Log_directory is writable.
*/
--- 493,498 ----
***************
*** 1188,1193 ****
--- 1183,1193 ----
filename = palloc(MAXPGPATH);
+ /*
+ * Create log directory if not present; ignore errors
+ */
+ CreateDir(Log_directory, 0700);
+
snprintf(filename, MAXPGPATH, "%s/", Log_directory);
len = strlen(filename);
Fujii Masao <masao.fujii@gmail.com> writes:
Here is the revised patch; If stats_temp_directory indicates the symlink,
we pursue the chain of symlinks and create the referenced directory.
I looked at this patch a bit. I'm still entirely unconvinced that we
should be doing this at all --- if the directory is not there, there's
a significant probability that there's something wrong that is beyond
the backend's ability to understand or correct. However, even ignoring
that objection, the patch is not ready to commit for a number of
reasons:
* Repeating the operation every time the stats file is written doesn't
seem like a particularly good idea; it eats cycles, and if the directory
disappears during live operation then there is *definitely* something
fishy going on. Can't we fix it so that the work is only done when the
path setting changes? (In principle you could do it in
assign_pgstat_temp_directory(), but I think something would be needed to
ensure that only the stats collector process actually tries to create
the directory. Or maybe it would be simplest to try to run the code only
when we get a failure from trying to create the stats temp file.)
* I don't think the mkdir_p code belongs in fd.c. It looks like
you copied-and-pasted it from initdb.c, which isn't any good either;
we don't want to maintain multiple copies of this. Maybe a new
src/port/ file is indicated.
* elog(LOG) is not exactly an adequate response if the final chdir fails
--- you have just broken the process beyond recovery. That alone may be
sufficient reason to reject the attempt to deal with symlinks. As far
as pgstat_temp_directory is concerned, I'm not sure of the point of
making the GUC point to a symlink anyway --- if you have a GUC why not
just point it where you want the directory to be?regards, tom lane
Tom Lane wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
Here is the revised patch; If stats_temp_directory indicates the symlink,
we pursue the chain of symlinks and create the referenced directory.I looked at this patch a bit. I'm still entirely unconvinced that we
should be doing this at all --- if the directory is not there, there's
a significant probability that there's something wrong that is beyond
the backend's ability to understand or correct. However, even ignoring
that objection, the patch is not ready to commit for a number of
reasons:* Repeating the operation every time the stats file is written doesn't
seem like a particularly good idea; it eats cycles, and if the directory
disappears during live operation then there is *definitely* something
fishy going on. Can't we fix it so that the work is only done when the
path setting changes? (In principle you could do it in
assign_pgstat_temp_directory(), but I think something would be needed to
ensure that only the stats collector process actually tries to create
the directory. Or maybe it would be simplest to try to run the code only
when we get a failure from trying to create the stats temp file.)
My idea was to have it run when it tries to create the temp file and
fails. Seems simpler than in the assign hook.
* I don't think the mkdir_p code belongs in fd.c. It looks like
you copied-and-pasted it from initdb.c, which isn't any good either;
we don't want to maintain multiple copies of this. Maybe a new
src/port/ file is indicated.
Yes, that's what I thought as well. How about src/port/dirmod.c though -
it has rename/unlink/symlink now which seem at least remotely related.
//Magnus
I assume we decided we didn't want this.
---------------------------------------------------------------------------
Tom Lane wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
Here is the revised patch; If stats_temp_directory indicates the symlink,
we pursue the chain of symlinks and create the referenced directory.I looked at this patch a bit. I'm still entirely unconvinced that we
should be doing this at all --- if the directory is not there, there's
a significant probability that there's something wrong that is beyond
the backend's ability to understand or correct. However, even ignoring
that objection, the patch is not ready to commit for a number of
reasons:* Repeating the operation every time the stats file is written doesn't
seem like a particularly good idea; it eats cycles, and if the directory
disappears during live operation then there is *definitely* something
fishy going on. Can't we fix it so that the work is only done when the
path setting changes? (In principle you could do it in
assign_pgstat_temp_directory(), but I think something would be needed to
ensure that only the stats collector process actually tries to create
the directory. Or maybe it would be simplest to try to run the code only
when we get a failure from trying to create the stats temp file.)* I don't think the mkdir_p code belongs in fd.c. It looks like
you copied-and-pasted it from initdb.c, which isn't any good either;
we don't want to maintain multiple copies of this. Maybe a new
src/port/ file is indicated.* elog(LOG) is not exactly an adequate response if the final chdir fails --- you have just broken the process beyond recovery. That alone may be sufficient reason to reject the attempt to deal with symlinks. As far as pgstat_temp_directory is concerned, I'm not sure of the point of making the GUC point to a symlink anyway --- if you have a GUC why not just point it where you want the directory to be?regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +Bruce Momjian <bruce@momjian.us> writes:
I assume we decided we didn't want this.
I thought the risk/reward ratio was pretty bad.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
I assume we decided we didn't want this.
I thought the risk/reward ratio was pretty bad.
Yea, the symlink issue killed it for me.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +