Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-20 Thread Alvaro Herrera
Tom Lane wrote:
 Andres Freund and...@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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
*** 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 

Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Alvaro Herrera
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
*** 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,
  			

Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Alvaro Herrera
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services
*** 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,

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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Tom Lane
Alvaro Herrera alvhe...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Tom Lane
Alvaro Herrera alvhe...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Andres Freund
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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Tom Lane
Andres Freund and...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Andres Freund
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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Tom Lane
Andres Freund and...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Andres Freund
On 2013-08-19 15:25:38 -0400, Tom Lane wrote:
 Andres Freund and...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Josh Berkus
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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Andres Freund
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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Tom Lane
Josh Berkus j...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Andres Freund
On 2013-08-19 15:51:16 -0400, Tom Lane wrote:
 Josh Berkus j...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Tom Lane
Andres Freund and...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Alvaro Herrera
Tom Lane wrote:
 Alvaro Herrera alvhe...@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 Herrerahttp://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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Tom Lane
Alvaro Herrera alvhe...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Jeff Janes
On Monday, August 19, 2013, Alvaro Herrera wrote:

 Tom Lane wrote:
  Alvaro Herrera alvhe...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-19 Thread Alvaro Herrera
Tom Lane wrote:
 Alvaro Herrera alvhe...@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 Herrerahttp://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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-15 Thread Magnus Hagander
On Aug 15, 2013 3:44 AM, Tom Lane t...@sss.pgh.pa.us wrote:

 Josh Berkus j...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-14 Thread Josh Berkus
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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-14 Thread Tom Lane
Josh Berkus j...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-14 Thread Alvaro Herrera
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 Herrerahttp://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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-13 Thread Jeff Janes
On Thu, Apr 25, 2013 at 8:24 AM, Peter Eisentraut pete...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-13 Thread Josh Berkus
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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-08-13 Thread Jeff Janes
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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-04-26 Thread Robert Haas
On Thu, Apr 25, 2013 at 12:09 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Alvaro Herrera alvhe...@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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-04-25 Thread Peter Eisentraut
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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-04-25 Thread Andrew Dunstan


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


[HACKERS] danger of stats_temp_directory = /dev/shm

2013-04-24 Thread Jeff Janes
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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-04-24 Thread Alvaro Herrera
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 Herrerahttp://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


Re: [HACKERS] danger of stats_temp_directory = /dev/shm

2013-04-24 Thread Tom Lane
Alvaro Herrera alvhe...@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