Fix syslogger so that log_truncate_on_rotation works in the first rotation.

tglsfdc · tglsfdc · commit 118b941614ce · 2012-07-31T14:37:04.000-04:00
In the original coding of the log rotation stuff, we did not bother to make
the truncation logic work for the very first rotation after postmaster
start (or after a syslogger crash and restart).  It just always appended
in that case.  It did not seem terribly important at the time, but we've
recently had two separate complaints from people who expected it to work
unsurprisingly.  (Both users tend to restart the postmaster about as often
as a log rotation is configured to happen, which is maybe not typical use,
but still...)  Since the initial log file is opened in the postmaster,
fixing this requires passing down some more state to the syslogger child
process.

It's always been like this, so back-patch to all supported branches.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
@@ -436,6 +436,7 @@ typedef struct
 	pid_t		PostmasterPid;
 	TimestampTz PgStartTime;
 	TimestampTz PgReloadTime;
+	pg_time_t	first_syslogger_file_time;
 	bool		redirection_done;
 	bool		IsBinaryUpgrade;
 #ifdef WIN32
@@ -4696,7 +4697,7 @@ MaxLivePostmasterChildren(void)
 
 /*
  * The following need to be available to the save/restore_backend_variables
- * functions
+ * functions.  They are marked NON_EXEC_STATIC in their home modules.
  */
 extern slock_t *ShmemLock;
 extern LWLock *LWLockArray;
@@ -4705,6 +4706,7 @@ extern PROC_HDR *ProcGlobal;
 extern PGPROC *AuxiliaryProcs;
 extern PMSignalData *PMSignalState;
 extern pgsocket pgStatSock;
+extern pg_time_t first_syslogger_file_time;
 
 #ifndef WIN32
 #define write_inheritable_socket(dest, src, childpid) ((*(dest) = (src)), true)
@@ -4756,6 +4758,7 @@ save_backend_variables(BackendParameters *param, Port *port,
 	param->PostmasterPid = PostmasterPid;
 	param->PgStartTime = PgStartTime;
 	param->PgReloadTime = PgReloadTime;
+	param->first_syslogger_file_time = first_syslogger_file_time;
 
 	param->redirection_done = redirection_done;
 	param->IsBinaryUpgrade = IsBinaryUpgrade;
@@ -4975,6 +4978,7 @@ restore_backend_variables(BackendParameters *param, Port *port)
 	PostmasterPid = param->PostmasterPid;
 	PgStartTime = param->PgStartTime;
 	PgReloadTime = param->PgReloadTime;
+	first_syslogger_file_time = param->first_syslogger_file_time;
 
 	redirection_done = param->redirection_done;
 	IsBinaryUpgrade = param->IsBinaryUpgrade;
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
@@ -2,7 +2,7 @@
  *
  * syslogger.c
  *
- * The system logger (syslogger) is new in Postgres 8.0. It catches all
+ * The system logger (syslogger) appeared in Postgres 8.0. It catches all
  * stderr output from the postmaster, backends, and other subprocesses
  * by redirecting to a pipe, and writes it to a set of logfiles.
  * It's possible to have size and age limits for the logfile configured
@@ -91,6 +91,7 @@ static bool pipe_eof_seen = false;
 static bool rotation_disabled = false;
 static FILE *syslogFile = NULL;
 static FILE *csvlogFile = NULL;
+NON_EXEC_STATIC pg_time_t first_syslogger_file_time = 0;
 static char *last_file_name = NULL;
 static char *last_csv_file_name = NULL;
 
@@ -284,6 +285,13 @@ SysLoggerMain(int argc, char *argv[])
 		elog(FATAL, "could not create syslogger data transfer thread: %m");
 #endif   /* WIN32 */
 
+	/*
+	 * Remember active logfile's name.  We recompute this from the reference
+	 * time because passing down just the pg_time_t is a lot cheaper than
+	 * passing a whole file path in the EXEC_BACKEND case.
+	 */
+	last_file_name = logfile_getname(first_syslogger_file_time, NULL);
+
 	/* remember active logfile parameters */
 	currentLogDir = pstrdup(Log_directory);
 	currentLogFilename = pstrdup(Log_filename);
@@ -535,9 +543,18 @@ SysLogger_Start(void)
 
 	/*
 	 * The initial logfile is created right in the postmaster, to verify that
-	 * the Log_directory is writable.
+	 * the Log_directory is writable.  We save the reference time so that
+	 * the syslogger child process can recompute this file name.
+	 *
+	 * It might look a bit strange to re-do this during a syslogger restart,
+	 * but we must do so since the postmaster closed syslogFile after the
+	 * previous fork (and remembering that old file wouldn't be right anyway).
+	 * Note we always append here, we won't overwrite any existing file.  This
+	 * is consistent with the normal rules, because by definition this is not
+	 * a time-based rotation.
 	 */
-	filename = logfile_getname(time(NULL), NULL);
+	first_syslogger_file_time = time(NULL);
+	filename = logfile_getname(first_syslogger_file_time, NULL);
 
 	syslogFile = logfile_open(filename, "a", false);
 
@@ -1017,8 +1034,12 @@ pipeThread(void *arg)
 #endif   /* WIN32 */
 
 /*
- * open the csv log file - we do this opportunistically, because
+ * Open the csv log file - we do this opportunistically, because
  * we don't know if CSV logging will be wanted.
+ *
+ * This is only used the first time we open the csv log in a given syslogger
+ * process, not during rotations.  As with opening the main log file, we
+ * always append in this situation.
  */
 static void
 open_csvlogfile(void)
@@ -1029,7 +1050,10 @@ open_csvlogfile(void)
 
 	csvlogFile = logfile_open(filename, "a", false);
 
-	pfree(filename);
+	if (last_csv_file_name != NULL)		/* probably shouldn't happen */
+		pfree(last_csv_file_name);
+
+	last_csv_file_name = filename;
 }
 
 /*
@@ -1108,14 +1132,7 @@ logfile_rotate(bool time_based_rotation, int size_rotation_for)
 	 * elapsed time and not something else, and (c) the computed file name is
 	 * different from what we were previously logging into.
 	 *
-	 * Note: during the first rotation after forking off from the postmaster,
-	 * last_file_name will be NULL.  (We don't bother to set it in the
-	 * postmaster because it ain't gonna work in the EXEC_BACKEND case.) So we
-	 * will always append in that situation, even though truncating would
-	 * usually be safe.
-	 *
-	 * For consistency, we treat CSV logs the same even though they aren't
-	 * opened in the postmaster.
+	 * Note: last_file_name should never be NULL here, but if it is, append.
 	 */
 	if (time_based_rotation || (size_rotation_for & LOG_DESTINATION_STDERR))
 	{
