danger of stats_temp_directory = /dev/shm
With the stats file split patch 187492b6c2e8cafc5 introduced in 9.3dev, now
after a crash the postmaster will try to delete all files in the directory
stats_temp_directory. When that is just a subdirectory of PGDATA, this is
fine. But it seems rather hostile when it is set to a shared directory,
like the popular /dev/shm.
Previously, it would only try to delete the one file /dev/shm/pgstat.stat,
so the danger was much smaller.
If /dev/shm is used, this only comes into play when postgres has crashed
but the OS has not (If the OS has crashed, /dev/shm it will be empty anyway
when it comes back) so perhaps this is not such a large exposure.
The docs don't discuss the issue of what happens if stats_temp_directory is
set to a non-private (to PGDATA) directory. The docs also do not
explicitly recommend using /dev/shm, but there are plenty of examples of
that usage that come up on google (and no examples of using a private
subdirectory of /dev/shm rather than /dev/shm itself).
Does this need to be fixed, or at least documented?
Cheers,
Jeff
Jeff Janes escribió:
With the stats file split patch 187492b6c2e8cafc5 introduced in 9.3dev, now
after a crash the postmaster will try to delete all files in the directory
stats_temp_directory. When that is just a subdirectory of PGDATA, this is
fine. But it seems rather hostile when it is set to a shared directory,
like the popular /dev/shm.
Does this need to be fixed, or at least documented?
I think we need it fixed so that it only deletes the files matching a
well-known pattern. In fact, there's an XXX comment about this in the
code:
/* XXX should we try to ignore files other than the ones we write? */
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Jeff Janes escribi�:
With the stats file split patch 187492b6c2e8cafc5 introduced in 9.3dev, now
after a crash the postmaster will try to delete all files in the directory
stats_temp_directory. When that is just a subdirectory of PGDATA, this is
fine. But it seems rather hostile when it is set to a shared directory,
like the popular /dev/shm.
Does this need to be fixed, or at least documented?
I think we need it fixed so that it only deletes the files matching a
well-known pattern.
I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.
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
On 4/25/13 12:09 AM, Tom Lane wrote:
I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.
Yeah, the requirements should be similar to what initdb requires for
PGDATA and pg_xlog.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 04/25/2013 11:24 AM, Peter Eisentraut wrote:
On 4/25/13 12:09 AM, Tom Lane wrote:
I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.Yeah, the requirements should be similar to what initdb requires for
PGDATA and pg_xlog.
Right.
I do think that best practice suggests using a dedicated ram drive
rather than /dev/shm. Here's an fstab entry I have used at one client's
site:
tmpfs /var/lib/pgsql/stats_tmp tmpfs
size=5G,uid=postgres,gid=postgres 0 0
I guess if we put in the sort of restrictions being suggested above I'd
add a mode argument to the mount options.
(This drive might seem large, but total RAM on this machine is 512Gb.)
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Apr 25, 2013 at 12:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Jeff Janes escribió:
With the stats file split patch 187492b6c2e8cafc5 introduced in 9.3dev, now
after a crash the postmaster will try to delete all files in the directory
stats_temp_directory. When that is just a subdirectory of PGDATA, this is
fine. But it seems rather hostile when it is set to a shared directory,
like the popular /dev/shm.Does this need to be fixed, or at least documented?
I think we need it fixed so that it only deletes the files matching a
well-known pattern.I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.
Only deleting files matching the relevant pattern might not be a bad
idea either, though.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Apr 25, 2013 at 8:24 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
On 4/25/13 12:09 AM, Tom Lane wrote:
I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.Yeah, the requirements should be similar to what initdb requires for
PGDATA and pg_xlog.
Is this a blocker for 9.3?
If it is a concern of not what is deleted but rather that someone can
inject a poisoned stats file into the directory, does it need to be
back-patched all the way, as that could be done before the split
patch?
Cheers,
Jeff
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/13/2013 09:57 AM, Jeff Janes wrote:
Is this a blocker for 9.3?
Why would it be? This issue doesn't originate with 9.3.
If it is a concern of not what is deleted but rather that someone can
inject a poisoned stats file into the directory, does it need to be
back-patched all the way, as that could be done before the split
patch?
I'd say it's a backpatch. We'll need to warn the heck out of users, though.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM4502171f788f91b5922e05515aba43519904a4b08eabf0accc6588eb2112b04f1165f99ad9e80a9727a20c0e3da5c805@asav-2.01.com
On Tuesday, August 13, 2013, Josh Berkus wrote:
On 08/13/2013 09:57 AM, Jeff Janes wrote:
Is this a blocker for 9.3?
Why would it be? This issue doesn't originate with 9.3.
Before 9.3, it would delete one specific file from a potentially shared
directory. In 9.3 it deletes the entire contents of a potentially shared
directory. That is a massive expansion in the surface area for
unintentional deletion. If we will disallow using shared directories
before the time 9.3 is released, that would fix it one way, but I don't
know if that is the plan or not.
Cheers,
Jeff
Jeff,
Before 9.3, it would delete one specific file from a potentially shared
directory. In 9.3 it deletes the entire contents of a potentially shared
directory. That is a massive expansion in the surface area for
unintentional deletion. If we will disallow using shared directories
before the time 9.3 is released, that would fix it one way, but I don't
know if that is the plan or not.
I can't see doing that. I can see adding the requirement for 9.3, and
then documenting it though.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM109d137bfb083f043db18f379e9dadeb881acddcab7f170c8169510ef405b5405fbc93fb644e5242d726b0fb4343a74c@asav-1.01.com
Josh Berkus <josh@agliodbs.com> writes:
Before 9.3, it would delete one specific file from a potentially shared
directory. In 9.3 it deletes the entire contents of a potentially shared
directory. That is a massive expansion in the surface area for
unintentional deletion. If we will disallow using shared directories
before the time 9.3 is released, that would fix it one way, but I don't
know if that is the plan or not.
I can't see doing that. I can see adding the requirement for 9.3, and
then documenting it though.
I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory). I agree that
back-patching such a change to the older branches is probably not a good
plan. I can't quite parse what you say above, so I'm not sure if you're
fully agreeing with that position or not.
In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.
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
Tom Lane wrote:
I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory). I agree that
back-patching such a change to the older branches is probably not a good
plan. I can't quite parse what you say above, so I'm not sure if you're
fully agreeing with that position or not.In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.
I will look into this.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Aug 15, 2013 3:44 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
Josh Berkus <josh@agliodbs.com> writes:
Before 9.3, it would delete one specific file from a potentially shared
directory. In 9.3 it deletes the entire contents of a potentially
shared
directory. That is a massive expansion in the surface area for
unintentional deletion. If we will disallow using shared directories
before the time 9.3 is released, that would fix it one way, but I don't
know if that is the plan or not.I can't see doing that. I can see adding the requirement for 9.3, and
then documenting it though.I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory). I agree that
back-patching such a change to the older branches is probably not a good
+1
In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.
+1 on that as well. It shouldn't be that hard to do.
/Magnus
Tom Lane wrote:
I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory).
Not an easy thing to do, this. It should be done as a GUC check hook,
ISTM, but this doesn't work because the first time those are run we
haven't yet changed to the data directory, and so any relative path
(which the default value is) will cause the check to fail (I *assume*
setting an absolute path would work, but I haven't tried). We could
skip the check on the first run, and verify the directory separately in
PostmasterMain() after changing CWD, but I don't see any way to detect
that we're in the initial run of GUC processing. Any thoughts? Maybe
the idea of using a GUC check hook is flawed, but I don't think so
because we also need to verify a directory when the setting changes on
SIGHUP.
The implementation I chose for the actual check was to separate the
permission checks from checkDataDir() into src/port/pgcheckdir.c that
returns different error codes for each case; see first attachment.
This part seems pretty reasonable, except that the code should be in
src/common rather than src/port, but I believe the entire pgcheckdir.c
file should be moved.
In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.
This seems pretty simple to do; see second attachment. (It would delete
files named, "db_1234.tmpfoobar", that is, valid names with suffixes,
but I can't see that being a problem). (I haven't really tested this
part at all.)
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
check-dir-perms.patchtext/x-diff; charset=us-asciiDownload
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 1333,1405 **** getInstallationPaths(const char *argv0)
static void
checkDataDir(void)
{
char path[MAXPGPATH];
FILE *fp;
- struct stat stat_buf;
Assert(DataDir);
! if (stat(DataDir, &stat_buf) != 0)
{
! if (errno == ENOENT)
ereport(FATAL,
(errcode_for_file_access(),
errmsg("data directory \"%s\" does not exist",
DataDir)));
! else
ereport(FATAL,
(errcode_for_file_access(),
! errmsg("could not read permissions of directory \"%s\": %m",
! DataDir)));
}
- /* eventual chdir would fail anyway, but let's test ... */
- if (!S_ISDIR(stat_buf.st_mode))
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("specified data directory \"%s\" is not a directory",
- DataDir)));
-
- /*
- * Check that the directory belongs to my userid; if not, reject.
- *
- * This check is an essential part of the interlock that prevents two
- * postmasters from starting in the same directory (see CreateLockFile()).
- * Do not remove or weaken it.
- *
- * XXX can we safely enable this check on Windows?
- */
- #if !defined(WIN32) && !defined(__CYGWIN__)
- if (stat_buf.st_uid != geteuid())
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("data directory \"%s\" has wrong ownership",
- DataDir),
- errhint("The server must be started by the user that owns the data directory.")));
- #endif
-
- /*
- * Check if the directory has group or world access. If so, reject.
- *
- * It would be possible to allow weaker constraints (for example, allow
- * group access) but we cannot make a general assumption that that is
- * okay; for example there are platforms where nearly all users
- * customarily belong to the same group. Perhaps this test should be
- * configurable.
- *
- * XXX temporarily suppress check when on Windows, because there may not
- * be proper support for Unix-y file permissions. Need to think of a
- * reasonable check to apply on Windows.
- */
- #if !defined(WIN32) && !defined(__CYGWIN__)
- if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("data directory \"%s\" has group or world access",
- DataDir),
- errdetail("Permissions should be u=rwx (0700).")));
- #endif
-
/* Look for PG_VERSION before looking for pg_control */
ValidatePgVersion(DataDir);
--- 1333,1383 ----
static void
checkDataDir(void)
{
+ CheckDirErrcode err;
char path[MAXPGPATH];
FILE *fp;
Assert(DataDir);
! err = checkDirectoryPermissions(DataDir);
! switch (err)
{
! case CKDIR_OK:
! break;
! case CKDIR_NOTEXISTS:
ereport(FATAL,
(errcode_for_file_access(),
errmsg("data directory \"%s\" does not exist",
DataDir)));
! break;
! case CKDIR_CANTREADPERMS:
ereport(FATAL,
(errcode_for_file_access(),
! errmsg("could not read permissions of data directory \"%s\": %m",
! DataDir)));
! break;
! case CKDIR_NOTADIR:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("specified data directory \"%s\" is not a directory",
! DataDir)));
! break;
! case CKDIR_WRONGOWNER:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("data directory \"%s\" has wrong ownership",
! DataDir),
! errhint("The server must be started by the user that owns the data directory.")));
! break;
! case CKDIR_TOOACCESIBLE:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("data directory \"%s\" has group or world access",
! DataDir),
! errdetail("Permissions should be u=rwx (0700).")));
! break;
}
/* Look for PG_VERSION before looking for pg_control */
ValidatePgVersion(DataDir);
*** a/src/include/port.h
--- b/src/include/port.h
***************
*** 459,464 **** extern char *inet_net_ntop(int af, const void *src, int bits,
--- 459,475 ----
/* port/pgcheckdir.c */
extern int pg_check_dir(const char *dir);
+ typedef enum
+ {
+ CKDIR_OK,
+ CKDIR_NOTEXISTS,
+ CKDIR_CANTREADPERMS,
+ CKDIR_NOTADIR,
+ CKDIR_WRONGOWNER,
+ CKDIR_TOOACCESIBLE
+ } CheckDirErrcode;
+ extern CheckDirErrcode checkDirectoryPermissions(char *directory);
+
/* port/pgmkdirp.c */
extern int pg_mkdir_p(char *path, int omode);
*** a/src/port/pgcheckdir.c
--- b/src/port/pgcheckdir.c
***************
*** 14,19 ****
--- 14,22 ----
#include "c.h"
#include <dirent.h>
+ #include <sys/types.h>
+ #include <sys/stat.h>
+ #include <unistd.h>
/*
***************
*** 88,90 **** pg_check_dir(const char *dir)
--- 91,147 ----
return result;
}
+
+ /*
+ * Verify permissions of a directory
+ */
+ CheckDirErrcode
+ checkDirectoryPermissions(char *directory)
+ {
+ struct stat stat_buf;
+
+ if (stat(directory, &stat_buf) != 0)
+ {
+ if (errno == ENOENT)
+ return CKDIR_NOTEXISTS;
+ else
+ return CKDIR_CANTREADPERMS;
+ }
+
+ if (!S_ISDIR(stat_buf.st_mode))
+ return CKDIR_NOTADIR;
+
+ /*
+ * Check that the directory belongs to my userid; if not, reject.
+ *
+ * This check is an essential part of the interlock that prevents two
+ * postmasters from starting in the same directory (see CreateLockFile()).
+ * Do not remove or weaken it.
+ *
+ * XXX can we safely enable this check on Windows?
+ */
+ #if !defined(WIN32) && !defined(__CYGWIN__)
+ if (stat_buf.st_uid != geteuid())
+ return CKDIR_WRONGOWNER;
+ #endif
+
+ /*
+ * Check if the directory has group or world access. If so, reject.
+ *
+ * It would be possible to allow weaker constraints (for example, allow
+ * group access) but we cannot make a general assumption that that is
+ * okay; for example there are platforms where nearly all users
+ * customarily belong to the same group. Perhaps this test should be
+ * configurable.
+ *
+ * XXX temporarily suppress check when on Windows, because there may not
+ * be proper support for Unix-y file permissions. Need to think of a
+ * reasonable check to apply on Windows.
+ */
+ #if !defined(WIN32) && !defined(__CYGWIN__)
+ if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+ return CKDIR_TOOACCESIBLE;
+ #endif
+
+ return CKDIR_OK;
+ }
Alvaro Herrera wrote:
In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.This seems pretty simple to do; see second attachment. (It would delete
files named, "db_1234.tmpfoobar", that is, valid names with suffixes,
but I can't see that being a problem). (I haven't really tested this
part at all.)
Here's the second attachment.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
skip-unknown-files.patchtext/x-diff; charset=us-asciiDownload
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
***************
*** 562,576 **** pgstat_reset_remove_files(const char *directory)
struct dirent *entry;
char fname[MAXPGPATH];
! dir = AllocateDir(pgstat_stat_directory);
! while ((entry = ReadDir(dir, pgstat_stat_directory)) != NULL)
{
if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0)
continue;
! /* XXX should we try to ignore files other than the ones we write? */
! snprintf(fname, MAXPGPATH, "%s/%s", pgstat_stat_directory,
entry->d_name);
unlink(fname);
}
--- 562,589 ----
struct dirent *entry;
char fname[MAXPGPATH];
! dir = AllocateDir(directory);
! while ((entry = ReadDir(dir, directory)) != NULL)
{
+ Oid tmp_oid;
+ char tmp_type[5];
+ int nitems;
+
if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0)
continue;
! /*
! * Skip directory entries that don't match the file names we write.
! * See get_dbstat_filename for the pattern.
! */
! nitems = sscanf(fname, "db_%u.%4s", &tmp_oid, tmp_type);
! if (nitems != 2)
! continue;
! if (strncmp(tmp_type, "tmp", 4) != 0 &&
! strncmp(tmp_type, "stat", 4) != 0)
! continue;
! snprintf(fname, MAXPGPATH, "%s/%s", directory,
entry->d_name);
unlink(fname);
}
***************
*** 3627,3632 **** get_dbstat_filename(bool permanent, bool tempname, Oid databaseid,
--- 3640,3646 ----
{
int printed;
+ /* NB -- pgstat_reset_remove_files knows about the pattern this uses */
printed = snprintf(filename, len, "%s/db_%u.%s",
permanent ? PGSTAT_STAT_PERMANENT_DIRECTORY :
pgstat_stat_directory,
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Alvaro Herrera wrote:
In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.
This seems pretty simple to do; see second attachment. (It would delete
files named, "db_1234.tmpfoobar", that is, valid names with suffixes,
but I can't see that being a problem). (I haven't really tested this
part at all.)
Here's the second attachment.
This looks good except that it can't tell "db_123.statfoo" isn't a match.
The scan limit/buffer size needs to be greater than the longest string
you care about, not only equal to. I think strcmp not strncmp would be
better coding, too. Please fix that and commit -- I think this part
is pretty noncontroversial.
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
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
The implementation I chose for the actual check was to separate the
permission checks from checkDataDir() into src/port/pgcheckdir.c that
returns different error codes for each case; see first attachment.
This part seems pretty reasonable, except that the code should be in
src/common rather than src/port, but I believe the entire pgcheckdir.c
file should be moved.
s/CKDIR_TOOACCESIBLE/CKDIR_TOOACCESSIBLE/, and maybe use underscores
to separate the words in those names? Otherwise no objection. But
there's not much point in this unless we can figure out where to call
it from for the stat_directory case.
One possibility is to do the initial check somewhere shortly after
ChangeToDataDir(), and have the GUC check hook only attempt to make a
check in SIGHUP context. Unfortunately we aren't passing the context to
check hooks, only GucSource which isn't adequate for this. Not sure if we
want to go so far as to change the check-hook API at this point. We could
probably think of some other, klugy way to tell if it's initial startup.
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
On 2013-08-19 14:28:28 -0400, Tom Lane wrote:
One possibility is to do the initial check somewhere shortly after
ChangeToDataDir(), and have the GUC check hook only attempt to make a
check in SIGHUP context. Unfortunately we aren't passing the context to
check hooks, only GucSource which isn't adequate for this. Not sure if we
want to go so far as to change the check-hook API at this point. We could
probably think of some other, klugy way to tell if it's initial startup.
Is it even actually safe to have stats_temp_directory PGC_SIGHUP after
the per DB splitup? I haven't audited the code for it, but it seems
somewhat likely that we would end up with some files in the old and some
in the new directory?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
On 2013-08-19 14:28:28 -0400, Tom Lane wrote:
One possibility is to do the initial check somewhere shortly after
ChangeToDataDir(), and have the GUC check hook only attempt to make a
check in SIGHUP context. Unfortunately we aren't passing the context to
check hooks, only GucSource which isn't adequate for this. Not sure if we
want to go so far as to change the check-hook API at this point. We could
probably think of some other, klugy way to tell if it's initial startup.
Is it even actually safe to have stats_temp_directory PGC_SIGHUP after
the per DB splitup? I haven't audited the code for it, but it seems
somewhat likely that we would end up with some files in the old and some
in the new directory?
That's a good point. For the moment we could just change it to
PGC_POSTMASTER and eliminate this problem. Reducing it back to SIGHUP
would be a future feature, which is evidently less than trivial.
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
On 2013-08-19 13:50:38 -0400, Alvaro Herrera wrote:
Tom Lane wrote:
I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory).Not an easy thing to do, this. It should be done as a GUC check hook,
ISTM, but this doesn't work because the first time those are run we
haven't yet changed to the data directory, and so any relative path
(which the default value is) will cause the check to fail (I *assume*
setting an absolute path would work, but I haven't tried). We could
skip the check on the first run, and verify the directory separately in
PostmasterMain() after changing CWD, but I don't see any way to detect
that we're in the initial run of GUC processing. Any thoughts? Maybe
the idea of using a GUC check hook is flawed, but I don't think so
because we also need to verify a directory when the setting changes on
SIGHUP.
Hm. Is a check like that actually sufficient? The idea of setting
stats_temp_directory to /dev/shm/postgres or similar in all of several
clusters on one machine doesn't seem to be that far fetched.
The only idea I have to prevent that is writing some minimal pg_control
like file into the temp stats directory iff it's empty. Then, when
reusing a stats temp directory, refuse to work unless it has the same
ids.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
Hm. Is a check like that actually sufficient? The idea of setting
stats_temp_directory to /dev/shm/postgres or similar in all of several
clusters on one machine doesn't seem to be that far fetched.
Hm. We could fairly easily have the lockfile management code also
write a postmaster.pid file into the stats_temp_directory, thus claiming
it in the same way as we do the $PGDATA dir itself. Not sure it's worth
the trouble though, since we've not heard any field reports of this sort
of thing.
I note BTW that similar complaints could be lodged against the
log_directory setting. We've not worried about that one too much.
Maybe we're overreacting to this issue for stats_temp_directory,
and tightening up the deletion code is a sufficient fix.
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
On 2013-08-19 15:25:38 -0400, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
Hm. Is a check like that actually sufficient? The idea of setting
stats_temp_directory to /dev/shm/postgres or similar in all of several
clusters on one machine doesn't seem to be that far fetched.Hm. We could fairly easily have the lockfile management code also
write a postmaster.pid file into the stats_temp_directory, thus claiming
it in the same way as we do the $PGDATA dir itself. Not sure it's worth
the trouble though, since we've not heard any field reports of this sort
of thing.
The reason I think it's more likely is that pg_stats_directory, to be
useful, really needs something like a ramdisk/tmpfs. Which is annoying
to create in a persistent fashion...
Very likely doing so would cause hard to diagnose planner issues using
completely absurd statistics. Not sure how often it would get properly
diagnosed.
But:
Maybe we're overreacting to this issue for stats_temp_directory,
and tightening up the deletion code is a sufficient fix.
You very well might be right. Just wanted to raise the issue.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom,
I note BTW that similar complaints could be lodged against the
log_directory setting. We've not worried about that one too much.
Actually, it does happen that when you change log_directory on a reload,
stuff takes an uneven amount of time to "cut over"; that is, there's a
few seconds while you're writing to both logs at once. Materially,
though, this isn't a serious operational issue (the logs are known to be
asynchronous), so beyond confusing newbies, it's not something we'd want
to fix.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WMf985a0df5a3c46892a7c66894fd439999a530e0e6cad5d14712a9c4dfc62761b1412f58ea60e6365b19b006585faf695@asav-1.01.com
Josh Berkus <josh@agliodbs.com> writes:
Tom,
I note BTW that similar complaints could be lodged against the
log_directory setting. We've not worried about that one too much.
Actually, it does happen that when you change log_directory on a reload,
stuff takes an uneven amount of time to "cut over"; that is, there's a
few seconds while you're writing to both logs at once. Materially,
though, this isn't a serious operational issue (the logs are known to be
asynchronous), so beyond confusing newbies, it's not something we'd want
to fix.
Yeah, the stats temp directory will act pretty much the same way: there
will be an interval where backends might not get the most up-to-date data,
but it's not clear that it's worth the trouble to get it to be more nearly
synchronous.
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
On 2013-08-19 12:47:05 -0700, Josh Berkus wrote:
Tom,
I note BTW that similar complaints could be lodged against the
log_directory setting. We've not worried about that one too much.Actually, it does happen that when you change log_directory on a reload,
stuff takes an uneven amount of time to "cut over"; that is, there's a
few seconds while you're writing to both logs at once. Materially,
though, this isn't a serious operational issue (the logs are known to be
asynchronous), so beyond confusing newbies, it's not something we'd want
to fix.
I think Tom was talking about the issue I noted on upthread which is
that two different clusters could write to the same
temp_stats_directory.
I've seen the logfile equivalent happen, but it was pretty easy to
diagnose...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-08-19 15:51:16 -0400, Tom Lane wrote:
Josh Berkus <josh@agliodbs.com> writes:
Tom,
I note BTW that similar complaints could be lodged against the
log_directory setting. We've not worried about that one too much.Actually, it does happen that when you change log_directory on a reload,
stuff takes an uneven amount of time to "cut over"; that is, there's a
few seconds while you're writing to both logs at once. Materially,
though, this isn't a serious operational issue (the logs are known to be
asynchronous), so beyond confusing newbies, it's not something we'd want
to fix.Yeah, the stats temp directory will act pretty much the same way: there
will be an interval where backends might not get the most up-to-date data,
but it's not clear that it's worth the trouble to get it to be more nearly
synchronous.
Won't it possibly cause stats being entirely lost?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
On 2013-08-19 15:51:16 -0400, Tom Lane wrote:
Yeah, the stats temp directory will act pretty much the same way: there
will be an interval where backends might not get the most up-to-date data,
but it's not clear that it's worth the trouble to get it to be more nearly
synchronous.
Won't it possibly cause stats being entirely lost?
How would that happen? The directory is write-only as far as the stats
collector is concerned, no?
Admittedly it might take a long time for it to write out the data again
for some database that wasn't getting any updates. Possibly it'd be worth
teaching the SIGHUP code segment in the stats collector to check for a
change in the value of stats_temp_directory and schedule write-out for all
databases if that happens.
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
Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Here's the second attachment.
This looks good except that it can't tell "db_123.statfoo" isn't a match.
The scan limit/buffer size needs to be greater than the longest string
you care about, not only equal to. I think strcmp not strncmp would be
better coding, too. Please fix that and commit -- I think this part
is pretty noncontroversial.
Pushed with those fixes, thanks. I noticed we also needed to match
files global.stat and global.tmp. Also I added another conversion to
the sscanf pattern, to ignore files named "db_1234.tmp.foo" and such
(i.e. stuff after whitespace).
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Pushed with those fixes, thanks. I noticed we also needed to match
files global.stat and global.tmp. Also I added another conversion to
the sscanf pattern, to ignore files named "db_1234.tmp.foo" and such
(i.e. stuff after whitespace).
After reading the sscanf man page, it seemed like this still wasn't
covering all the bases about where sscanf will silently skip whitespace,
so I hacked it a bit more.
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
On Monday, August 19, 2013, Alvaro Herrera wrote:
Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com <javascript:;>> writes:
Here's the second attachment.
This looks good except that it can't tell "db_123.statfoo" isn't a match.
The scan limit/buffer size needs to be greater than the longest string
you care about, not only equal to. I think strcmp not strncmp would be
better coding, too. Please fix that and commit -- I think this part
is pretty noncontroversial.Pushed with those fixes, thanks. I noticed we also needed to match
files global.stat and global.tmp. Also I added another conversion to
the sscanf pattern, to ignore files named "db_1234.tmp.foo" and such
(i.e. stuff after whitespace).
Thanks. I did not attack this with malice, but I did throw some casual
stupidity at it (setting the temp directory to the same as the absolute
path of the data directory) and it managed to go through crash recovery
successfully, while HEAD^ completely destroyed itself.
One oddity I observed is that the first time I tested it (by doing kill -9
on the checkpointer) it failed to come back up automatically, claiming the
start up process had been signalled with 9 during recovery. When I
manually restarted, it ran through recovery again and started successfully.
However I could not repeat this and can't see how this patch could
possibly cause a regression of this nature, so I'm going to chalk it up to
some bizarre race condition, or operator error.
Cheers,
Jeff
Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Pushed with those fixes, thanks. I noticed we also needed to match
files global.stat and global.tmp. Also I added another conversion to
the sscanf pattern, to ignore files named "db_1234.tmp.foo" and such
(i.e. stuff after whitespace).After reading the sscanf man page, it seemed like this still wasn't
covering all the bases about where sscanf will silently skip whitespace,
so I hacked it a bit more.
Funnily enough, my own reading of that manpage got me a flawed
understanding of %n -- I interpreted it as returning the number of items
matched, not the number of chars read .. and I was precisely looking for
a way to determine the latter. Thanks.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
On 2013-08-19 14:28:28 -0400, Tom Lane wrote:
One possibility is to do the initial check somewhere shortly after
ChangeToDataDir(), and have the GUC check hook only attempt to make a
check in SIGHUP context. Unfortunately we aren't passing the context to
check hooks, only GucSource which isn't adequate for this. Not sure if we
want to go so far as to change the check-hook API at this point. We could
probably think of some other, klugy way to tell if it's initial startup.Is it even actually safe to have stats_temp_directory PGC_SIGHUP after
the per DB splitup? I haven't audited the code for it, but it seems
somewhat likely that we would end up with some files in the old and some
in the new directory?That's a good point. For the moment we could just change it to
PGC_POSTMASTER and eliminate this problem. Reducing it back to SIGHUP
would be a future feature, which is evidently less than trivial.
Here's a patchset for this. The first patch is the same as upthread,
with the enum members renamed; the second is the actual pgstats change.
(I considered the idea that checkDirectoryPermissions returned a bitmask
of the failed checks, instead of just returning a code for the first
check that fails. This might be useful if some caller wants to ignore
certain problems. But at the moment I didn't see many other places
where this could be used, so it's probably pointless ATM.)
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
stats-temp-dir-1.patchtext/x-diff; charset=us-asciiDownload
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 1313,1385 **** getInstallationPaths(const char *argv0)
static void
checkDataDir(void)
{
char path[MAXPGPATH];
FILE *fp;
- struct stat stat_buf;
Assert(DataDir);
! if (stat(DataDir, &stat_buf) != 0)
{
! if (errno == ENOENT)
ereport(FATAL,
(errcode_for_file_access(),
errmsg("data directory \"%s\" does not exist",
DataDir)));
! else
ereport(FATAL,
(errcode_for_file_access(),
! errmsg("could not read permissions of directory \"%s\": %m",
! DataDir)));
}
- /* eventual chdir would fail anyway, but let's test ... */
- if (!S_ISDIR(stat_buf.st_mode))
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("specified data directory \"%s\" is not a directory",
- DataDir)));
-
- /*
- * Check that the directory belongs to my userid; if not, reject.
- *
- * This check is an essential part of the interlock that prevents two
- * postmasters from starting in the same directory (see CreateLockFile()).
- * Do not remove or weaken it.
- *
- * XXX can we safely enable this check on Windows?
- */
- #if !defined(WIN32) && !defined(__CYGWIN__)
- if (stat_buf.st_uid != geteuid())
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("data directory \"%s\" has wrong ownership",
- DataDir),
- errhint("The server must be started by the user that owns the data directory.")));
- #endif
-
- /*
- * Check if the directory has group or world access. If so, reject.
- *
- * It would be possible to allow weaker constraints (for example, allow
- * group access) but we cannot make a general assumption that that is
- * okay; for example there are platforms where nearly all users
- * customarily belong to the same group. Perhaps this test should be
- * configurable.
- *
- * XXX temporarily suppress check when on Windows, because there may not
- * be proper support for Unix-y file permissions. Need to think of a
- * reasonable check to apply on Windows.
- */
- #if !defined(WIN32) && !defined(__CYGWIN__)
- if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("data directory \"%s\" has group or world access",
- DataDir),
- errdetail("Permissions should be u=rwx (0700).")));
- #endif
-
/* Look for PG_VERSION before looking for pg_control */
ValidatePgVersion(DataDir);
--- 1313,1363 ----
static void
checkDataDir(void)
{
+ CheckDirErrcode err;
char path[MAXPGPATH];
FILE *fp;
Assert(DataDir);
! err = checkDirectoryPermissions(DataDir);
! switch (err)
{
! case CKDIR_OK:
! break;
! case CKDIR_NOT_EXISTS:
ereport(FATAL,
(errcode_for_file_access(),
errmsg("data directory \"%s\" does not exist",
DataDir)));
! break;
! case CKDIR_CANT_READ_PERMS:
ereport(FATAL,
(errcode_for_file_access(),
! errmsg("could not read permissions of data directory \"%s\": %m",
! DataDir)));
! break;
! case CKDIR_NOT_DIR:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("specified data directory \"%s\" is not a directory",
! DataDir)));
! break;
! case CKDIR_WRONG_OWNER:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("data directory \"%s\" has wrong ownership",
! DataDir),
! errhint("The server must be started by the user that owns the data directory.")));
! break;
! case CKDIR_TOO_ACCESSIBLE:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("data directory \"%s\" has group or world access",
! DataDir),
! errdetail("Permissions should be u=rwx (0700).")));
! break;
}
/* Look for PG_VERSION before looking for pg_control */
ValidatePgVersion(DataDir);
*** a/src/include/port.h
--- b/src/include/port.h
***************
*** 463,468 **** extern char *inet_net_ntop(int af, const void *src, int bits,
--- 463,479 ----
/* port/pgcheckdir.c */
extern int pg_check_dir(const char *dir);
+ typedef enum
+ {
+ CKDIR_OK,
+ CKDIR_NOT_EXISTS,
+ CKDIR_CANT_READ_PERMS,
+ CKDIR_NOT_DIR,
+ CKDIR_WRONG_OWNER,
+ CKDIR_TOO_ACCESSIBLE
+ } CheckDirErrcode;
+ extern CheckDirErrcode checkDirectoryPermissions(char *directory);
+
/* port/pgmkdirp.c */
extern int pg_mkdir_p(char *path, int omode);
*** a/src/port/pgcheckdir.c
--- b/src/port/pgcheckdir.c
***************
*** 14,19 ****
--- 14,22 ----
#include "c.h"
#include <dirent.h>
+ #include <sys/types.h>
+ #include <sys/stat.h>
+ #include <unistd.h>
/*
***************
*** 88,90 **** pg_check_dir(const char *dir)
--- 91,147 ----
return result;
}
+
+ /*
+ * Verify permissions of a directory
+ */
+ CheckDirErrcode
+ checkDirectoryPermissions(char *directory)
+ {
+ struct stat stat_buf;
+
+ if (stat(directory, &stat_buf) != 0)
+ {
+ if (errno == ENOENT)
+ return CKDIR_NOT_EXISTS;
+ else
+ return CKDIR_CANT_READ_PERMS;
+ }
+
+ if (!S_ISDIR(stat_buf.st_mode))
+ return CKDIR_NOT_DIR;
+
+ /*
+ * Check that the directory belongs to my userid; if not, reject.
+ *
+ * This check is an essential part of the interlock that prevents two
+ * postmasters from starting in the same directory (see CreateLockFile()).
+ * Do not remove or weaken it.
+ *
+ * XXX can we safely enable this check on Windows?
+ */
+ #if !defined(WIN32) && !defined(__CYGWIN__)
+ if (stat_buf.st_uid != geteuid())
+ return CKDIR_WRONG_OWNER;
+ #endif
+
+ /*
+ * Check if the directory has group or world access. If so, reject.
+ *
+ * It would be possible to allow weaker constraints (for example, allow
+ * group access) but we cannot make a general assumption that that is
+ * okay; for example there are platforms where nearly all users
+ * customarily belong to the same group. Perhaps this test should be
+ * configurable.
+ *
+ * XXX temporarily suppress check when on Windows, because there may not
+ * be proper support for Unix-y file permissions. Need to think of a
+ * reasonable check to apply on Windows.
+ */
+ #if !defined(WIN32) && !defined(__CYGWIN__)
+ if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+ return CKDIR_TOO_ACCESSIBLE;
+ #endif
+
+ return CKDIR_OK;
+ }
stats-temp-dir-2.patchtext/x-diff; charset=us-asciiDownload
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 4446,4453 **** COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
is <filename>pg_stat_tmp</filename>. Pointing this at a RAM-based
file system will decrease physical I/O requirements and can lead to
improved performance.
! This parameter can only be set in the <filename>postgresql.conf</>
! file or on the server command line.
</para>
</listitem>
</varlistentry>
--- 4446,4452 ----
is <filename>pg_stat_tmp</filename>. Pointing this at a RAM-based
file system will decrease physical I/O requirements and can lead to
improved performance.
! This parameter can only be set at server start.
</para>
</listitem>
</varlistentry>
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
***************
*** 554,559 **** startup_failed:
--- 554,628 ----
}
/*
+ * Verify the permissions of stats_temp_directory.
+ *
+ * For security reasons, we want this directory to be tightly controlled;
+ * both so that we don't interfere with other running instances, and so that
+ * others can't inject fake stats.
+ *
+ * The obvious way to do this is to set this function as a GUC check hook; but
+ * this doesn't work because the first time those hooks are run, we have not
+ * changed the current directory to the data directory yet; so the check fails
+ * when it's set to a relative path, as the default value is. We'd have to
+ * find a way to skip this check in the first run of check hooks. To avoid this
+ * problem, the stats_temp_directory setting is PGC_POSTMASTER for now, and we
+ * run the check separately in PostmasterMain, after changing directory,
+ * instead.
+ *
+ * If we were to overcome that problem, we should probably also consider having
+ * the stats collector write a full set of files in the new temp directory if
+ * the setting is changed on SIGHUP.
+ */
+ bool
+ check_pgstat_temp_dir_perms(char *dir)
+ {
+ int elevel = FATAL;
+ CheckDirErrcode err;
+
+ err = checkDirectoryPermissions(dir);
+
+ switch (err)
+ {
+ case CKDIR_OK:
+ break;
+ case CKDIR_NOT_EXISTS:
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("stats_temp_directory \"%s\" does not exist",
+ dir)));
+ break;
+ case CKDIR_CANT_READ_PERMS:
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not read permissions of stats_temp_directory \"%s\": %m",
+ dir)));
+ break;
+ case CKDIR_NOT_DIR:
+ ereport(elevel,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("specified stats_temp_directory \"%s\" is not a directory",
+ dir)));
+ break;
+ case CKDIR_WRONG_OWNER:
+ ereport(elevel,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("stats_temp_directory \"%s\" has wrong ownership",
+ dir),
+ errhint("The server must be started by the user that owns the stats_temp_directory.")));
+ break;
+ case CKDIR_TOO_ACCESSIBLE:
+ ereport(elevel,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("stats_temp_directory \"%s\" has group or world access",
+ dir),
+ errdetail("Permissions should be u=rwx (0700).")));
+ break;
+ }
+
+ return err == CKDIR_OK;
+ }
+
+ /*
* subroutine for pgstat_reset_all
*/
static void
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 829,834 **** PostmasterMain(int argc, char *argv[])
--- 829,836 ----
ExitPostmaster(1);
}
+ (void) check_pgstat_temp_dir_perms(pgstat_stat_directory);
+
/*
* Now that we are done processing the postmaster arguments, reset
* getopt(3) library so that it will work correctly in subprocesses.
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 3081,3087 **** static struct config_string ConfigureNamesString[] =
},
{
! {"stats_temp_directory", PGC_SIGHUP, STATS_COLLECTOR,
gettext_noop("Writes temporary statistics files to the specified directory."),
NULL,
GUC_SUPERUSER_ONLY
--- 3081,3088 ----
},
{
! /* see check_pgstat_temp_dir_perms for why this is PGC_POSTMASTER */
! {"stats_temp_directory", PGC_POSTMASTER, STATS_COLLECTOR,
gettext_noop("Writes temporary statistics files to the specified directory."),
NULL,
GUC_SUPERUSER_ONLY
*** a/src/include/pgstat.h
--- b/src/include/pgstat.h
***************
*** 750,755 **** extern void pgstat_init(void);
--- 750,756 ----
extern int pgstat_start(void);
extern void pgstat_reset_all(void);
extern void allow_immediate_pgstat_restart(void);
+ extern bool check_pgstat_temp_dir_perms(char *dir);
#ifdef EXEC_BACKEND
extern void PgstatCollectorMain(int argc, char *argv[]) __attribute__((noreturn));