logfile_rotate(): correct multiple calls to set_next_rotation_time()

Because set_next_rotation_time() was being called once per call to
logfile_rotate(), the timestamp used to create the log file name was
being advanced multiple times per rotation. This led to the creation of
empty log files on disk that looked like they were from "the future".

This code badly needs to be refactored -- the initial intent, for the
text and csv implementations to be independent, is no longer the case --
but for now, we can hack around this problem by pulling the call to
set_next_rotation_time() up one level. This will only be called if *all*
logfiles are correctly rotated, to match the upstream logic (on failure,
we loop around to the beginning and try to open all log files again).
Co-authored-by: NJamie McAtamney <jmcatamney@pivotal.io>

src/backend/postmaster/syslogger.c

@@ -172,7 +172,7 @@ static FILE *logfile_open(const char *filename, const char *mode,
 #ifdef WIN32
 static unsigned int __stdcall pipeThread(void *arg);
 #endif
-static void logfile_rotate(bool time_based_rotation, bool size_based_rotation, const char *suffix,
+static bool logfile_rotate(bool time_based_rotation, bool size_based_rotation, const char *suffix,
 						   const char *log_directory, const char *log_filename,
                            FILE **fh, char **last_log_file_name);
 static char *logfile_getname(pg_time_t timestamp, const char *suffix, const char *log_directory, const char *log_file_pattern);
@@ -431,6 +431,8 @@ SysLoggerMain(int argc, char *argv[])
 		int			rc;
 #endif
 
+		bool		all_rotations_occurred = false;
+
 		/* Clear any already-pending wakeups */
 		ResetLatch(&sysLoggerLatch);
 
@@ -534,6 +536,9 @@ SysLoggerMain(int argc, char *argv[])
 			}
 		}
 
+		all_rotations_occurred = rotation_requested ||
+								 (alert_log_level_opened && alert_rotation_requested);
+
 		if (rotation_requested)
 		{
 			/*
@@ -544,20 +549,35 @@ SysLoggerMain(int argc, char *argv[])
 				size_rotation_for = LOG_DESTINATION_STDERR | LOG_DESTINATION_CSVLOG;
 
 			rotation_requested = false;
-			logfile_rotate(time_based_rotation, (size_rotation_for & LOG_DESTINATION_STDERR) != 0,
-						   NULL, Log_directory, Log_filename,
-						   &syslogFile, &last_file_name);
-			logfile_rotate(time_based_rotation, (size_rotation_for & LOG_DESTINATION_CSVLOG) != 0,
-						   ".csv", Log_directory, Log_filename,
-						   &csvlogFile, &last_csv_file_name);
+
+			all_rotations_occurred &=
+				logfile_rotate(time_based_rotation, (size_rotation_for & LOG_DESTINATION_STDERR) != 0,
+							   NULL, Log_directory, Log_filename,
+							   &syslogFile, &last_file_name);
+			all_rotations_occurred &=
+				logfile_rotate(time_based_rotation, (size_rotation_for & LOG_DESTINATION_CSVLOG) != 0,
+							   ".csv", Log_directory, Log_filename,
+							   &csvlogFile, &last_csv_file_name);
 		}
 
 		if (alert_log_level_opened && alert_rotation_requested)
 		{
 			alert_rotation_requested = false;
-			logfile_rotate(time_based_rotation, size_rotation_for_alert,
-						   NULL, gp_perf_mon_directory, alert_file_pattern,
-						   &alertLogFile, &alert_last_file_name);
+			all_rotations_occurred &=
+				logfile_rotate(time_based_rotation, size_rotation_for_alert,
+							   NULL, gp_perf_mon_directory, alert_file_pattern,
+							   &alertLogFile, &alert_last_file_name);
+		}
+
+		/*
+		 * GPDB: only update our rotation timestamp if every log file above was
+		 * able to rotate. In upstream, this would have been done as part of
+		 * logfile_rotate() itself -- Postgres calls that function once, whereas
+		 * we call it (up to) three times.
+		 */
+		if (all_rotations_occurred)
+		{
+			set_next_rotation_time();
 		}
 
 		/*
@@ -567,6 +587,9 @@ SysLoggerMain(int argc, char *argv[])
 		 * until after calling logfile_rotate(), since it will advance
 		 * next_rotation_time.
 		 *
+		 * GPDB: logfile_rotate() doesn't advance next_rotation_time; we do that
+		 * explicitly above, once all rotations have been successful.
+		 *
 		 * Also note that we need to beware of overflow in calculation of the
 		 * timeout: with large settings of Log_RotationAge, next_rotation_time
 		 * could be more than INT_MAX msec in the future.  In that case we'll
@@ -2361,7 +2384,6 @@ logfile_open(const char *filename, const char *mode, bool allow_errors)
 /*
  * perform logfile rotation.
  *
- *
  * In GPDB, this has been modified significantly from the upstream version:
  *
  * - In PostgreSQL, one call to logfile_rotate performs rotation for both the
@@ -2369,11 +2391,12 @@ logfile_open(const char *filename, const char *mode, bool allow_errors)
  *   and also for the GPDB specific 'alert' log
  * - In PostgreSQL, this resets 'rotation_requested' flag. In GPDB, the caller
  *   has to do it.
- *
- *
- *
+ * - In PostgreSQL, this calls set_next_rotation_time(). In GPDB, the caller
+ *   has to do it once all calls to this function return true (i.e. after all
+ *   rotations have been successfully completed for the current timestamp), to
+ *   avoid having the filename timestamp advance multiple times per rotation.
  */
-static void
+static bool
 logfile_rotate(bool time_based_rotation, bool size_based_rotation,
 			   const char *suffix,
                const char *log_directory, 
@@ -2433,7 +2456,7 @@ logfile_rotate(bool time_based_rotation, bool size_based_rotation,
 
 			if (filename)
 				pfree(filename);
-			return;
+			return false;
 		}
 
 		if (*fh_p)
@@ -2517,7 +2540,7 @@ logfile_rotate(bool time_based_rotation, bool size_based_rotation,
 	if (filename)
 		pfree(filename);
 
-	set_next_rotation_time();
+	return true;
 }