Code & docs review for server instrumentation patch. File timestamps

should surely be timestamptz not timestamp; fix some but not all of the
holes in check_and_make_absolute(); other minor cleanup.  Also put in
the missed catversion bump.

src/backend/postmaster/postmaster.c

@@ -37,7 +37,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/postmaster/postmaster.c,v 1.463 2005/08/12 03:23:51 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/postmaster/postmaster.c,v 1.464 2005/08/12 18:23:53 tgl Exp $
  *
  * NOTES
  *
@@ -3382,9 +3382,8 @@ sigusr1_handler(SIGNAL_ARGS)
 		}
 	}
 
-	if (PgArchPID != 0 && Shutdown == NoShutdown)
-	{
-		if (CheckPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER))
+	if (CheckPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER) &&
+		PgArchPID != 0 && Shutdown == NoShutdown)
 	{
 		/*
 		 * Send SIGUSR1 to archiver process, to wake it up and begin
@@ -3392,10 +3391,13 @@ sigusr1_handler(SIGNAL_ARGS)
 		 */
 		kill(PgArchPID, SIGUSR1);
 	}
-	}
 
-	if (CheckPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE) && SysLoggerPID != 0)
+	if (CheckPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE) &&
+		SysLoggerPID != 0)
+	{
+		/* Tell syslogger to rotate logfile */
 	    kill(SysLoggerPID, SIGUSR1);
+	}
 
 	PG_SETMASK(&UnBlockSig);
 

src/backend/utils/adt/genfile.c

 /*-------------------------------------------------------------------------
  *
  * genfile.c
+ *		Functions for direct access to files
  *
  *
- * Copyright (c) 2004, PostgreSQL Global Development Group
+ * Copyright (c) 2004-2005, PostgreSQL Global Development Group
  * 
  * Author: Andreas Pflug <pgadmin@pse-consulting.de>
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/adt/genfile.c,v 1.1 2005/08/12 03:24:08 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/adt/genfile.c,v 1.2 2005/08/12 18:23:54 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -19,13 +20,15 @@
 #include <unistd.h>
 #include <dirent.h>
 
-#include "utils/builtins.h"
-#include "miscadmin.h"
-#include "storage/fd.h"
+#include "access/heapam.h"
 #include "catalog/pg_type.h"
 #include "funcapi.h"
+#include "miscadmin.h"
+#include "postmaster/syslogger.h"
+#include "storage/fd.h"
+#include "utils/builtins.h"
+#include "utils/memutils.h"
 
-extern  char *Log_directory;
 
 typedef struct 
 {
@@ -33,13 +36,16 @@ typedef struct
 	DIR		*dirdesc;
 } directory_fctx;
 
+
 /*
- * Return an absolute path. Argument may be absolute or 
- * relative to the DataDir.
+ * Validate a path and convert to absolute form.
+ *
+ * Argument may be absolute or relative to the DataDir (but we only allow
+ * absolute paths that match Log_directory).
  */
-static char *check_and_make_absolute(text *arg)
+static char *
+check_and_make_absolute(text *arg)
 {
-	int datadir_len = strlen(DataDir);
 	int filename_len = VARSIZE(arg) - VARHDRSZ;
 	char *filename = palloc(filename_len + 1);
 	
@@ -52,16 +58,21 @@ static char *check_and_make_absolute(text *arg)
 	/*
 	 *	Prevent reference to the parent directory.
 	 *	"..a.." is a valid file name though.
+	 *
+	 * XXX this is BROKEN because it fails to prevent "C:.." on Windows.
+	 * Need access to "skip_drive" functionality to do it right.  (There
+	 * is no actual security hole because we'll prepend the DataDir below,
+	 * resulting in a just-plain-broken path, but we should give the right
+	 * error message instead.)
 	 */
-	if (strcmp(filename, "..") == 0 ||							/* beginning */
-		strncmp(filename, "../", 3) == 0 ||						/* beginning */
-		strcmp(filename, "/..") == 0 ||							/* beginning */
+	if (strcmp(filename, "..") == 0 ||						/* whole */
 		strncmp(filename, "../", 3) == 0 ||					/* beginning */
 		strstr(filename, "/../") != NULL ||					/* middle */
-		strncmp(filename + filename_len - 3, "/..", 3) == 0)	/* end */
+		(filename_len >= 3 &&
+		 strcmp(filename + filename_len - 3, "/..") == 0))	/* end */
 			ereport(ERROR,
 				  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				   (errmsg("Reference to a parent directory (\"..\") not allowed"))));
+				   (errmsg("reference to parent directory (\"..\") not allowed"))));
 
 	if (is_absolute_path(filename))
 	{
@@ -74,12 +85,12 @@ static char *check_and_make_absolute(text *arg)
 
 	    ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 (errmsg("Absolute paths not allowed"))));
-		return NULL;
+				 (errmsg("absolute path not allowed"))));
+		return NULL;			/* keep compiler quiet */
 	}
 	else
 	{
-	    char *absname = palloc(datadir_len + filename_len + 2);
+	    char *absname = palloc(strlen(DataDir) + filename_len + 2);
 		sprintf(absname, "%s/%s", DataDir, filename);
 		pfree(filename);
 		return absname;
@@ -87,11 +98,16 @@ static char *check_and_make_absolute(text *arg)
 }
 
 
-Datum pg_read_file(PG_FUNCTION_ARGS)
+/*
+ * Read a section of a file, returning it as text
+ */
+Datum
+pg_read_file(PG_FUNCTION_ARGS)
 {
-	int64		bytes_to_read = PG_GETARG_INT64(2);
+	text	   *filename_t = PG_GETARG_TEXT_P(0);
 	int64		seek_offset = PG_GETARG_INT64(1);
-	char 		*buf = 0;
+	int64		bytes_to_read = PG_GETARG_INT64(2);
+	char 		*buf;
 	size_t		nbytes;
 	FILE		*file;
 	char		*filename;
@@ -101,107 +117,108 @@ Datum pg_read_file(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 (errmsg("must be superuser to read files"))));
 
-	filename = check_and_make_absolute(PG_GETARG_TEXT_P(0));
+	filename = check_and_make_absolute(filename_t);
 
 	if ((file = AllocateFile(filename, PG_BINARY_R)) == NULL)
-	{
 		ereport(ERROR,
 				(errcode_for_file_access(),
-				 errmsg("could not open file %s for reading: %m", filename)));
-		PG_RETURN_NULL();
-	}
+				 errmsg("could not open file \"%s\" for reading: %m",
+						filename)));
 
-	if (fseeko(file, (off_t)seek_offset,
+	if (fseeko(file, (off_t) seek_offset,
 			   (seek_offset >= 0) ? SEEK_SET : SEEK_END) != 0)
-	{
 		ereport(ERROR,
 				(errcode_for_file_access(),
-				 errmsg("could not seek in file %s: %m", filename)));
-		PG_RETURN_NULL();
-	}
+				 errmsg("could not seek in file \"%s\": %m", filename)));
 
 	if (bytes_to_read < 0)
-	{
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("length cannot be negative")));
-	}
+				 errmsg("requested length cannot be negative")));
+
+	/* not sure why anyone thought that int64 length was a good idea */
+	if (bytes_to_read > (MaxAllocSize - VARHDRSZ))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("requested length too large")));
 	
-	buf = palloc(bytes_to_read + VARHDRSZ);
+	buf = palloc((Size) bytes_to_read + VARHDRSZ);
 
-	nbytes = fread(VARDATA(buf), 1, bytes_to_read, file);
+	nbytes = fread(VARDATA(buf), 1, (size_t) bytes_to_read, file);
 
 	if (nbytes < 0)
-	{
 		ereport(ERROR,
 				(errcode_for_file_access(),
-				 errmsg("could not read file %s: %m", filename)));
-		PG_RETURN_NULL();
-	}
+				 errmsg("could not read file \"%s\": %m", filename)));
+
 	VARATT_SIZEP(buf) = nbytes + VARHDRSZ;
 
-	pfree(filename);
 	FreeFile(file);
+	pfree(filename);
+
 	PG_RETURN_TEXT_P(buf);
 }
 
-
-Datum pg_stat_file(PG_FUNCTION_ARGS)
+/*
+ * stat a file
+ */
+Datum
+pg_stat_file(PG_FUNCTION_ARGS)
 {
-	AttInMetadata *attinmeta;
-	char		*filename = check_and_make_absolute(PG_GETARG_TEXT_P(0));
+	text	   *filename_t = PG_GETARG_TEXT_P(0);
+	char		*filename;
 	struct stat fst;
-	char		lenbuf[30], cbuf[30], abuf[30], mbuf[30], dirbuf[2];
-	char		*values[5] = {lenbuf, cbuf, abuf, mbuf, dirbuf};
-	pg_time_t	timestamp;
+	Datum		values[5];
+	bool		isnull[5];
 	HeapTuple	tuple;
-	TupleDesc	tupdesc = CreateTemplateTupleDesc(5, false);
+	TupleDesc	tupdesc;
 
 	if (!superuser())
 	    ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 (errmsg("must be superuser to get file information"))));
 
-	TupleDescInitEntry(tupdesc, (AttrNumber) 1, "length", INT8OID, -1, 0);
-	TupleDescInitEntry(tupdesc, (AttrNumber) 2, "atime", TIMESTAMPOID, -1, 0);
-	TupleDescInitEntry(tupdesc, (AttrNumber) 3, "mtime", TIMESTAMPOID, -1, 0);
-	TupleDescInitEntry(tupdesc, (AttrNumber) 4, "ctime", TIMESTAMPOID, -1, 0);
-	TupleDescInitEntry(tupdesc, (AttrNumber) 5, "isdir", BOOLOID, -1, 0);
-	attinmeta = TupleDescGetAttInMetadata(tupdesc);
+	filename = check_and_make_absolute(filename_t);
 
 	if (stat(filename, &fst) < 0)
-	{
 		ereport(ERROR,
 				(errcode_for_file_access(),
-				 errmsg("could not stat file %s: %m", filename)));
-		PG_RETURN_NULL();
-	}
-	else
-	{
-		snprintf(lenbuf, 30, INT64_FORMAT, (int64)fst.st_size);
+				 errmsg("could not stat file \"%s\": %m", filename)));
+
+	tupdesc = CreateTemplateTupleDesc(5, false);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 1,
+					   "length", INT8OID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 2,
+					   "atime", TIMESTAMPTZOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 3,
+					   "mtime", TIMESTAMPTZOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 4,
+					   "ctime", TIMESTAMPTZOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 5,
+					   "isdir", BOOLOID, -1, 0);
+	BlessTupleDesc(tupdesc);
+
+	values[0] = Int64GetDatum((int64) fst.st_size);
+	values[1] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_atime));
+	values[2] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_mtime));
+	values[3] = TimestampTzGetDatum(time_t_to_timestamptz(fst.st_ctime));
+	values[4] = BoolGetDatum(fst.st_mode & S_IFDIR);
+
+	memset(isnull, false, sizeof(isnull));
+
+	tuple = heap_form_tuple(tupdesc, values, isnull);
 
-		timestamp = fst.st_atime;
-		pg_strftime(abuf, 30, "%F %T", pg_localtime(&timestamp, global_timezone));
-
-		timestamp = fst.st_mtime;
-		pg_strftime(mbuf, 30, "%F %T", pg_localtime(&timestamp, global_timezone));
-
-		timestamp = fst.st_ctime;
-		pg_strftime(cbuf, 30, "%F %T", pg_localtime(&timestamp, global_timezone));
-
-		if (fst.st_mode & S_IFDIR)
-			strcpy(dirbuf, "t");
-		else
-			strcpy(dirbuf, "f");
-
-		tuple = BuildTupleFromCStrings(attinmeta, values);
 	pfree(filename);
+
 	PG_RETURN_DATUM(HeapTupleGetDatum(tuple));
-	}
 }
 
 
-Datum pg_ls_dir(PG_FUNCTION_ARGS)
+/*
+ * List a directory (returns the filenames only)
+ */
+Datum
+pg_ls_dir(PG_FUNCTION_ARGS)
 {
 	FuncCallContext	*funcctx;
 	struct dirent	*de;
@@ -227,7 +244,8 @@ Datum pg_ls_dir(PG_FUNCTION_ARGS)
 		if (!fctx->dirdesc)
 		    ereport(ERROR,
 					(errcode_for_file_access(),
-					 errmsg("%s is not browsable: %m", fctx->location)));
+					 errmsg("could not open directory \"%s\": %m",
+							fctx->location)));
 
 		funcctx->user_fctx = fctx;
 		MemoryContextSwitchTo(oldcontext);
@@ -236,17 +254,16 @@ Datum pg_ls_dir(PG_FUNCTION_ARGS)
 	funcctx = SRF_PERCALL_SETUP();
 	fctx = (directory_fctx*) funcctx->user_fctx;
 
-	if (!fctx->dirdesc)  /* not a readable directory  */
-		SRF_RETURN_DONE(funcctx);
-
 	while ((de = ReadDir(fctx->dirdesc, fctx->location)) != NULL)
 	{
 		int			len = strlen(de->d_name);
-		text		*result = palloc(len + VARHDRSZ);
+		text		*result;
 
-		if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0)
+		if (strcmp(de->d_name, ".") == 0 ||
+			strcmp(de->d_name, "..") == 0)
 		    continue;
 
+		result = palloc(len + VARHDRSZ);
 		VARATT_SIZEP(result) = len + VARHDRSZ;
 		memcpy(VARDATA(result), de->d_name, len);
 
@@ -254,5 +271,6 @@ Datum pg_ls_dir(PG_FUNCTION_ARGS)
 	}
 
 	FreeDir(fctx->dirdesc);
+
 	SRF_RETURN_DONE(funcctx);
 }

src/backend/utils/adt/misc.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/adt/misc.c,v 1.46 2005/08/12 03:24:08 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/adt/misc.c,v 1.47 2005/08/12 18:23:54 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -18,26 +18,19 @@
 #include <signal.h>
 #include <dirent.h>
 
+#include "catalog/pg_tablespace.h"
+#include "catalog/pg_type.h"
 #include "commands/dbcommands.h"
+#include "funcapi.h"
 #include "miscadmin.h"
-#include "storage/procarray.h"
-#include "storage/pmsignal.h"
+#include "postmaster/syslogger.h"
 #include "storage/fd.h"
+#include "storage/pmsignal.h"
+#include "storage/procarray.h"
 #include "utils/builtins.h"
-#include "utils/elog.h"
-#include "funcapi.h"
-#include "catalog/pg_type.h"
-#include "catalog/pg_tablespace.h"
-#include "postmaster/syslogger.h"
 
 #define atooid(x)  ((Oid) strtoul((x), NULL, 10))
 
-typedef struct 
-{
-	char	*location;
-	DIR		*dirdesc;
-} directory_fctx;
-
 
 /*
  * Check if data is Null
@@ -150,15 +143,15 @@ pg_rotate_logfile(PG_FUNCTION_ARGS)
 
 	if (!Redirect_stderr)
 	{
-	    ereport(NOTICE,
-				(errcode(ERRCODE_WARNING),
-				 errmsg("no logfile configured; rotation not supported")));
+		ereport(WARNING,
+				(errmsg("rotation not possible because log redirection not active")));
+
 		PG_RETURN_INT32(0);
 	}
 
 	SendPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE);
 
-	PG_RETURN_INT32(0);
+	PG_RETURN_INT32(1);
 }
 
 #ifdef NOT_USED

src/backend/utils/adt/timestamp.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/adt/timestamp.c,v 1.147 2005/07/30 18:20:44 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/adt/timestamp.c,v 1.148 2005/08/12 18:23:54 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -939,7 +939,7 @@ GetCurrentTimestamp(void)
 
 	gettimeofday(&tp, NULL);
 
-	result = tp.tv_sec -
+	result = (TimestampTz) tp.tv_sec -
 		((POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) * SECS_PER_DAY);
 
 #ifdef HAVE_INT64_TIMESTAMP
@@ -951,6 +951,29 @@ GetCurrentTimestamp(void)
 	return result;
 }
 
+
+/*
+ * Convert a time_t to TimestampTz.
+ *
+ * We do not use time_t internally in Postgres, but this is provided for use
+ * by functions that need to interpret, say, a stat(2) result.
+ */
+TimestampTz
+time_t_to_timestamptz(time_t tm)
+{
+	TimestampTz result;
+
+	result = (TimestampTz) tm -
+		((POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) * SECS_PER_DAY);
+
+#ifdef HAVE_INT64_TIMESTAMP
+	result *= USECS_PER_SEC;
+#endif
+
+	return result;
+}
+
+
 void
 dt2time(Timestamp jd, int *hour, int *min, int *sec, fsec_t *fsec)
 {

src/include/catalog/catversion.h

@@ -37,7 +37,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/catalog/catversion.h,v 1.296 2005/08/11 21:11:47 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/catalog/catversion.h,v 1.297 2005/08/12 18:23:55 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -53,6 +53,6 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	200508111
+#define CATALOG_VERSION_NO	200508121
 
 #endif

src/include/catalog/pg_proc.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/catalog/pg_proc.h,v 1.381 2005/08/12 03:24:22 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/catalog/pg_proc.h,v 1.382 2005/08/12 18:23:55 tgl Exp $
  *
  * NOTES
  *	  The script catalog/genbki.sh reads this file and generates .bki
@@ -3058,12 +3058,12 @@ DESCR("Rotate log file");
 
 DATA(insert OID = 2623 ( pg_stat_file	    PGNSP PGUID 12 f f t f v 1 2249 "25" _null_ _null_ _null_ pg_stat_file - _null_ ));
 DESCR("Return file information");
-DATA(insert OID = 2624 ( pg_file_length	    PGNSP PGUID 14 f f t f v 1 20 "25" _null_ _null_ _null_ "SELECT len FROM pg_stat_file($1) AS s(len int8, c timestamp, a timestamp, m timestamp, i bool)" - _null_ ));
+DATA(insert OID = 2624 ( pg_file_length	    PGNSP PGUID 14 f f t f v 1 20 "25" _null_ _null_ _null_ "SELECT len FROM pg_stat_file($1) AS s(len int8, a timestamptz, m timestamptz, c timestamptz, i bool)" - _null_ ));
 DESCR("Return file length");
 DATA(insert OID = 2625 ( pg_read_file	    PGNSP PGUID 12 f f t f v 3 25 "25 20 20" _null_ _null_ _null_ pg_read_file - _null_ ));
 DESCR("Read text from a file");
 DATA(insert OID = 2626 ( pg_ls_dir		    PGNSP PGUID 12 f f t t v 1 25 "25" _null_ _null_ _null_ pg_ls_dir - _null_ ));
-DESCR("List all file in a directory");
+DESCR("List all files in a directory");
 
   
 /* Aggregates (moved here from pg_aggregate for 7.3) */

src/include/utils/timestamp.h

@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/timestamp.h,v 1.53 2005/07/22 05:08:26 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/utils/timestamp.h,v 1.54 2005/08/12 18:23:56 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -292,6 +292,8 @@ extern Datum pgsql_postmaster_start_time(PG_FUNCTION_ARGS);
 
 extern TimestampTz GetCurrentTimestamp(void);
 
+extern TimestampTz time_t_to_timestamptz(time_t tm);
+
 extern int	tm2timestamp(struct pg_tm *tm, fsec_t fsec, int *tzp, Timestamp *dt);
 extern int timestamp2tm(Timestamp dt, int *tzp, struct pg_tm *tm,
 			 fsec_t *fsec, char **tzn, pg_tz *attimezone);