Refactor per-destination file rotation in logging collector

stderr and csvlog have been using duplicated code when it came to the
rotation of their file by size, age or if forced by a user request
(pg_ctl logrotate or the SQL function pg_rotate_logfile).  The main
difference between both is that stderr requires its file to always be
opened, so as it is possible to have a redirection route if the logging
collector is not ready yet to do its work if alternate destinations are
enabled.

Also, if csvlog gets disabled, we need to close properly its meta-data
stored in the logging collector (last file name for current_logfiles and
fd currently open for business).  Except for those points, the code is
the same in terms of error handling and if a file should be created or
just continued.

This change makes the code simpler overall, and it will help in the
introduction of more file-based log destinations.  This refactoring is
similar to the work done in 5b0b699.  Most of the duplication originates
from fd801f4.

Some of the TAP tests of pg_ctl check the case of a forced log rotation,
but this is somewhat limited as there is no coverage for
log_rotation_age or log_rotation_size (these may not be worth the extra
resources to run either), and no coverage for reload of log_destination
with different combinations of stderr and csvlog.  I have tested all
those cases separately for this refactoring.

Author: Michael Paquier
Discussion: https://postgr.es/m/CAH7T-aqswBM6JWe4pDehi1uOiufqe06DJWaU5=X7dDLyqUExHg@mail.gmail.com

Author: michaelpq

src/backend/postmaster/syslogger.c

@@ -87,7 +87,7 @@ 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_sys_file_name = NULL;
 static char *last_csv_file_name = NULL;
 
 /*
@@ -145,6 +145,10 @@ static FILE *logfile_open(const char *filename, const char *mode,
 static unsigned int __stdcall pipeThread(void *arg);
 #endif
 static void logfile_rotate(bool time_based_rotation, int size_rotation_for);
+static bool logfile_rotate_dest(bool time_based_rotation,
+								int size_rotation_for, pg_time_t fntime,
+								int target_dest, char **last_file_name,
+								FILE **logFile);
 static char *logfile_getname(pg_time_t timestamp, const char *suffix);
 static void set_next_rotation_time(void);
 static void sigUsr1Handler(SIGNAL_ARGS);
@@ -274,7 +278,7 @@ SysLoggerMain(int argc, char *argv[])
 	 * 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);
+	last_sys_file_name = logfile_getname(first_syslogger_file_time, NULL);
 	if (csvlogFile != NULL)
 		last_csv_file_name = logfile_getname(first_syslogger_file_time, ".csv");
 
@@ -1242,145 +1246,138 @@ logfile_open(const char *filename, const char *mode, bool allow_errors)
 }
 
 /*
- * perform logfile rotation
+ * Do logfile rotation for a single destination, as specified by target_dest.
+ * The information stored in *last_file_name and *logFile is updated on a
+ * successful file rotation.
+ *
+ * Returns false if the rotation has been stopped, or true to move on to
+ * the processing of other formats.
  */
-static void
-logfile_rotate(bool time_based_rotation, int size_rotation_for)
+static bool
+logfile_rotate_dest(bool time_based_rotation, int size_rotation_for,
+					pg_time_t fntime, int target_dest,
+					char **last_file_name, FILE **logFile)
 {
+	char	   *logFileExt;
 	char	   *filename;
-	char	   *csvfilename = NULL;
-	pg_time_t	fntime;
 	FILE	   *fh;
 
-	rotation_requested = false;
+	/*
+	 * If the target destination was just turned off, close the previous file
+	 * and unregister its data.  This cannot happen for stderr as syslogFile
+	 * is assumed to be always opened even if stderr is disabled in
+	 * log_destination.
+	 */
+	if ((Log_destination & target_dest) == 0 &&
+		target_dest != LOG_DESTINATION_STDERR)
+	{
+		if (*logFile != NULL)
+			fclose(*logFile);
+		*logFile = NULL;
+		if (*last_file_name != NULL)
+			pfree(*last_file_name);
+		*last_file_name = NULL;
+		return true;
+	}
 
 	/*
-	 * When doing a time-based rotation, invent the new logfile name based on
-	 * the planned rotation time, not current time, to avoid "slippage" in the
-	 * file name when we don't do the rotation immediately.
+	 * Leave if it is not time for a rotation or if the target destination has
+	 * no need to do a rotation based on the size of its file.
 	 */
-	if (time_based_rotation)
-		fntime = next_rotation_time;
+	if (!time_based_rotation && (size_rotation_for & target_dest) == 0)
+		return true;
+
+	/* file extension depends on the destination type */
+	if (target_dest == LOG_DESTINATION_STDERR)
+		logFileExt = NULL;
+	else if (target_dest == LOG_DESTINATION_CSVLOG)
+		logFileExt = ".csv";
 	else
-		fntime = time(NULL);
-	filename = logfile_getname(fntime, NULL);
-	if (Log_destination & LOG_DESTINATION_CSVLOG)
-		csvfilename = logfile_getname(fntime, ".csv");
+	{
+		/* cannot happen */
+		Assert(false);
+	}
+
+	/* build the new file name */
+	filename = logfile_getname(fntime, logFileExt);
 
 	/*
 	 * Decide whether to overwrite or append.  We can overwrite if (a)
 	 * Log_truncate_on_rotation is set, (b) the rotation was triggered by
 	 * elapsed time and not something else, and (c) the computed file name is
 	 * different from what we were previously logging into.
-	 *
-	 * Note: last_file_name should never be NULL here, but if it is, append.
 	 */
-	if (time_based_rotation || (size_rotation_for & LOG_DESTINATION_STDERR))
-	{
-		if (Log_truncate_on_rotation && time_based_rotation &&
-			last_file_name != NULL &&
-			strcmp(filename, last_file_name) != 0)
-			fh = logfile_open(filename, "w", true);
-		else
-			fh = logfile_open(filename, "a", true);
+	if (Log_truncate_on_rotation && time_based_rotation &&
+		*last_file_name != NULL &&
+		strcmp(filename, *last_file_name) != 0)
+		fh = logfile_open(filename, "w", true);
+	else
+		fh = logfile_open(filename, "a", true);
 
-		if (!fh)
+	if (!fh)
+	{
+		/*
+		 * ENFILE/EMFILE are not too surprising on a busy system; just keep
+		 * using the old file till we manage to get a new one.  Otherwise,
+		 * assume something's wrong with Log_directory and stop trying to
+		 * create files.
+		 */
+		if (errno != ENFILE && errno != EMFILE)
 		{
-			/*
-			 * ENFILE/EMFILE are not too surprising on a busy system; just
-			 * keep using the old file till we manage to get a new one.
-			 * Otherwise, assume something's wrong with Log_directory and stop
-			 * trying to create files.
-			 */
-			if (errno != ENFILE && errno != EMFILE)
-			{
-				ereport(LOG,
-						(errmsg("disabling automatic rotation (use SIGHUP to re-enable)")));
-				rotation_disabled = true;
-			}
-
-			if (filename)
-				pfree(filename);
-			if (csvfilename)
-				pfree(csvfilename);
-			return;
+			ereport(LOG,
+					(errmsg("disabling automatic rotation (use SIGHUP to re-enable)")));
+			rotation_disabled = true;
 		}
 
-		fclose(syslogFile);
-		syslogFile = fh;
-
-		/* instead of pfree'ing filename, remember it for next time */
-		if (last_file_name != NULL)
-			pfree(last_file_name);
-		last_file_name = filename;
-		filename = NULL;
+		if (filename)
+			pfree(filename);
+		return false;
 	}
 
-	/*
-	 * Same as above, but for csv file.  Note that if LOG_DESTINATION_CSVLOG
-	 * was just turned on, we might have to open csvlogFile here though it was
-	 * not open before.  In such a case we'll append not overwrite (since
-	 * last_csv_file_name will be NULL); that is consistent with the normal
-	 * rules since it's not a time-based rotation.
-	 */
-	if ((Log_destination & LOG_DESTINATION_CSVLOG) &&
-		(csvlogFile == NULL ||
-		 time_based_rotation || (size_rotation_for & LOG_DESTINATION_CSVLOG)))
-	{
-		if (Log_truncate_on_rotation && time_based_rotation &&
-			last_csv_file_name != NULL &&
-			strcmp(csvfilename, last_csv_file_name) != 0)
-			fh = logfile_open(csvfilename, "w", true);
-		else
-			fh = logfile_open(csvfilename, "a", true);
+	/* fill in the new information */
+	if (*logFile != NULL)
+		fclose(*logFile);
+	*logFile = fh;
 
-		if (!fh)
-		{
-			/*
-			 * ENFILE/EMFILE are not too surprising on a busy system; just
-			 * keep using the old file till we manage to get a new one.
-			 * Otherwise, assume something's wrong with Log_directory and stop
-			 * trying to create files.
-			 */
-			if (errno != ENFILE && errno != EMFILE)
-			{
-				ereport(LOG,
-						(errmsg("disabling automatic rotation (use SIGHUP to re-enable)")));
-				rotation_disabled = true;
-			}
+	/* instead of pfree'ing filename, remember it for next time */
+	if (*last_file_name != NULL)
+		pfree(*last_file_name);
+	*last_file_name = filename;
 
-			if (filename)
-				pfree(filename);
-			if (csvfilename)
-				pfree(csvfilename);
-			return;
-		}
+	return true;
+}
+
+/*
+ * perform logfile rotation
+ */
+static void
+logfile_rotate(bool time_based_rotation, int size_rotation_for)
+{
+	pg_time_t	fntime;
 
-		if (csvlogFile != NULL)
-			fclose(csvlogFile);
-		csvlogFile = fh;
+	rotation_requested = false;
 
-		/* instead of pfree'ing filename, remember it for next time */
-		if (last_csv_file_name != NULL)
-			pfree(last_csv_file_name);
-		last_csv_file_name = csvfilename;
-		csvfilename = NULL;
-	}
-	else if (!(Log_destination & LOG_DESTINATION_CSVLOG) &&
-			 csvlogFile != NULL)
-	{
-		/* CSVLOG was just turned off, so close the old file */
-		fclose(csvlogFile);
-		csvlogFile = NULL;
-		if (last_csv_file_name != NULL)
-			pfree(last_csv_file_name);
-		last_csv_file_name = NULL;
-	}
+	/*
+	 * When doing a time-based rotation, invent the new logfile name based on
+	 * the planned rotation time, not current time, to avoid "slippage" in the
+	 * file name when we don't do the rotation immediately.
+	 */
+	if (time_based_rotation)
+		fntime = next_rotation_time;
+	else
+		fntime = time(NULL);
 
-	if (filename)
-		pfree(filename);
-	if (csvfilename)
-		pfree(csvfilename);
+	/* file rotation for stderr */
+	if (!logfile_rotate_dest(time_based_rotation, size_rotation_for, fntime,
+							 LOG_DESTINATION_STDERR, &last_sys_file_name,
+							 &syslogFile))
+		return;
+
+	/* file rotation for csvlog */
+	if (!logfile_rotate_dest(time_based_rotation, size_rotation_for, fntime,
+							 LOG_DESTINATION_CSVLOG, &last_csv_file_name,
+							 &csvlogFile))
+		return;
 
 	update_metainfo_datafile();
 
@@ -1501,9 +1498,9 @@ update_metainfo_datafile(void)
 		return;
 	}
 
-	if (last_file_name && (Log_destination & LOG_DESTINATION_STDERR))
+	if (last_sys_file_name && (Log_destination & LOG_DESTINATION_STDERR))
 	{
-		if (fprintf(fh, "stderr %s\n", last_file_name) < 0)
+		if (fprintf(fh, "stderr %s\n", last_sys_file_name) < 0)
 		{
 			ereport(LOG,
 					(errcode_for_file_access(),