On Fri, Sep 17, 2021 at 04:36:57PM -0400, Sehrope Sarkuni wrote: > It was helpful to split them out while working on the patch but I see your > point upon re-reading through the result. > > Attached version (renamed to 002) adds only three static functions for > checking rotation size, performing the actual rotation, and closing. Also > one other to handle generating the logfile names with a suffix to handle > the pfree-ing (wrapping logfile_open(...)). > > The rest of the changes happen in place using the new structures.
I have looked at that in details, and found that the introduction of FileLogDestination makes the code harder to follow, and that the introduction of the file extension, the destination name and the expected target destination LOG_DESTINATION_* had a limited impact because they are used in few places. The last two useful pieces are the FILE* handle and the last file name for current_logfiles. Attached are updated patches. The logic of 0001 to refactor the fd fetch/save logic when forking the syslogger in EXEC_BACKEND builds is unchanged. I have tweaked the patch with more comments and different routine names though. Patch 0002 refactors the main point that introduced FileLogDestination by refactoring the per-destination file rotation, not forgetting the fact that the file last name and handle for stderr can never be cleaned up even if LOG_DESTINATION_STDERR is disabled. Grepping after LOG_DESTINATION_CSVLOG in the code tree, I'd be fine to live with this level of abstraction as each per-destination change are grouped with each other so they are hard to miss. 0001 is in a rather commitable shape, and I have made the code consistent with HEAD. However, I think that its handling of _get_osfhandle() is clunky for 64-bit compilations as long is 32b in WIN32 but intptr_t is platform-dependent as it could be 32b or 64b, so atoi() would overflow if the handle is larger than INT_MAX for 64b builds: https://docs.microsoft.com/en-us/cpp/c-runtime-library/standard-types This problem deserves a different thread. It would be good for 0002 if an extra pair of eyes looks at it. While on it, I have renamed the existing last_file_name to last_sys_file_name in 0002 to make the naming more consistent with syslogFile. It is independent of 0001, so it could be done first as well. -- Michael
From 57bcaf49bd10ec3e6afb14f3ec384cb88a65efe6 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 28 Sep 2021 12:28:21 +0900
Subject: [PATCH v7 1/2] Refactor fd handling when forking syslogger in
EXEC_BACKEND build
---
src/backend/postmaster/syslogger.c | 122 +++++++++++++++--------------
1 file changed, 62 insertions(+), 60 deletions(-)
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index bca3883572..ddb47e3cb0 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -130,6 +130,8 @@ static volatile sig_atomic_t rotation_requested = false;
/* Local subroutines */
#ifdef EXEC_BACKEND
+static long syslogger_fdget(FILE *file);
+static FILE *syslogger_fdopen(int fd);
static pid_t syslogger_forkexec(void);
static void syslogger_parseArgs(int argc, char *argv[]);
#endif
@@ -733,6 +735,60 @@ SysLogger_Start(void)
#ifdef EXEC_BACKEND
+/*
+ * syslogger_fdget() -
+ *
+ * Utility wrapper to grab the file descriptor of an opened error output
+ * file. Used when building the command to fork the logging collector.
+ */
+static long
+syslogger_fdget(FILE *file)
+{
+#ifndef WIN32
+ if (file != NULL)
+ return (long) fileno(file);
+ else
+ return -1;
+#else /* WIN32 */
+ if (file != NULL)
+ return (long) _get_osfhandle(_fileno(file));
+ else
+ return 0;
+#endif /* WIN32 */
+}
+
+/*
+ * syslogger_fdopen() -
+ *
+ * Utility wrapper to re-open an error output file, using the given file
+ * descriptor. Used when parsing arguments in a forked logging collector.
+ */
+static FILE *
+syslogger_fdopen(int fd)
+{
+ FILE *file = NULL;
+
+#ifndef WIN32
+ if (fd != -1)
+ {
+ file = fdopen(fd, "a");
+ setvbuf(file, NULL, PG_IOLBF, 0);
+ }
+#else /* WIN32 */
+ if (fd != 0)
+ {
+ fd = _open_osfhandle(fd, _O_APPEND | _O_TEXT);
+ if (fd > 0)
+ {
+ file = fdopen(fd, "a");
+ setvbuf(file, NULL, PG_IOLBF, 0);
+ }
+ }
+#endif /* WIN32 */
+
+ return file;
+}
+
/*
* syslogger_forkexec() -
*
@@ -751,34 +807,11 @@ syslogger_forkexec(void)
av[ac++] = NULL; /* filled in by postmaster_forkexec */
/* static variables (those not passed by write_backend_variables) */
-#ifndef WIN32
- if (syslogFile != NULL)
- snprintf(filenobuf, sizeof(filenobuf), "%d",
- fileno(syslogFile));
- else
- strcpy(filenobuf, "-1");
-#else /* WIN32 */
- if (syslogFile != NULL)
- snprintf(filenobuf, sizeof(filenobuf), "%ld",
- (long) _get_osfhandle(_fileno(syslogFile)));
- else
- strcpy(filenobuf, "0");
-#endif /* WIN32 */
+ snprintf(filenobuf, sizeof(filenobuf), "%ld",
+ syslogger_fdget(syslogFile));
av[ac++] = filenobuf;
-
-#ifndef WIN32
- if (csvlogFile != NULL)
- snprintf(csvfilenobuf, sizeof(csvfilenobuf), "%d",
- fileno(csvlogFile));
- else
- strcpy(csvfilenobuf, "-1");
-#else /* WIN32 */
- if (csvlogFile != NULL)
- snprintf(csvfilenobuf, sizeof(csvfilenobuf), "%ld",
- (long) _get_osfhandle(_fileno(csvlogFile)));
- else
- strcpy(csvfilenobuf, "0");
-#endif /* WIN32 */
+ snprintf(csvfilenobuf, sizeof(csvfilenobuf), "%ld",
+ syslogger_fdget(csvlogFile));
av[ac++] = csvfilenobuf;
av[ac] = NULL;
@@ -807,41 +840,10 @@ syslogger_parseArgs(int argc, char *argv[])
* fails there's not a lot we can do to report the problem anyway. As
* coded, we'll just crash on a null pointer dereference after failure...
*/
-#ifndef WIN32
fd = atoi(*argv++);
- if (fd != -1)
- {
- syslogFile = fdopen(fd, "a");
- setvbuf(syslogFile, NULL, PG_IOLBF, 0);
- }
+ syslogFile = syslogger_fdopen(fd);
fd = atoi(*argv++);
- if (fd != -1)
- {
- csvlogFile = fdopen(fd, "a");
- setvbuf(csvlogFile, NULL, PG_IOLBF, 0);
- }
-#else /* WIN32 */
- fd = atoi(*argv++);
- if (fd != 0)
- {
- fd = _open_osfhandle(fd, _O_APPEND | _O_TEXT);
- if (fd > 0)
- {
- syslogFile = fdopen(fd, "a");
- setvbuf(syslogFile, NULL, PG_IOLBF, 0);
- }
- }
- fd = atoi(*argv++);
- if (fd != 0)
- {
- fd = _open_osfhandle(fd, _O_APPEND | _O_TEXT);
- if (fd > 0)
- {
- csvlogFile = fdopen(fd, "a");
- setvbuf(csvlogFile, NULL, PG_IOLBF, 0);
- }
- }
-#endif /* WIN32 */
+ csvlogFile = syslogger_fdopen(fd);
}
#endif /* EXEC_BACKEND */
--
2.33.0
From 88d43ad72b252e9efd2c9b7f5ba84d786ad78471 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 28 Sep 2021 11:48:18 +0900
Subject: [PATCH v7 2/2] Refactor per-destination file rotation in syslogger
---
src/backend/postmaster/syslogger.c | 242 ++++++++++++++---------------
1 file changed, 119 insertions(+), 123 deletions(-)
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index ddb47e3cb0..2c13e7ef9f 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/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;
/*
@@ -274,7 +274,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");
@@ -1241,16 +1241,118 @@ logfile_open(const char *filename, const char *mode, bool allow_errors)
return fh;
}
+/*
+ * 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, and true to move on to
+ * the processing of other formats.
+ */
+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;
+ FILE *fh;
+
+ /*
+ * 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
+ * LOG_DESTINATION_STDERR is disabled.
+ */
+ 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;
+ }
+
+ /*
+ * Leave if it is not time for a rotation or if the target destination
+ * has no need to do a rotation.
+ */
+ 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
+ {
+ /* 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.
+ */
+ 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)
+ {
+ /*
+ * 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);
+ return false;
+ }
+
+ /* fill in the new information */
+ if (*logFile != NULL)
+ fclose(*logFile);
+ *logFile = fh;
+
+ /* instead of pfree'ing filename, remember it for next time */
+ if (*last_file_name != NULL)
+ pfree(*last_file_name);
+ *last_file_name = filename;
+
+ return true;
+}
+
/*
* perform logfile rotation
*/
static void
logfile_rotate(bool time_based_rotation, int size_rotation_for)
{
- char *filename;
- char *csvfilename = NULL;
pg_time_t fntime;
- FILE *fh;
rotation_requested = false;
@@ -1263,124 +1365,18 @@ logfile_rotate(bool time_based_rotation, int size_rotation_for)
fntime = next_rotation_time;
else
fntime = time(NULL);
- filename = logfile_getname(fntime, NULL);
- if (Log_destination & LOG_DESTINATION_CSVLOG)
- csvfilename = logfile_getname(fntime, ".csv");
- /*
- * 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);
+ /* file rotation for stderr */
+ if (!logfile_rotate_dest(time_based_rotation, size_rotation_for, fntime,
+ LOG_DESTINATION_STDERR, &last_sys_file_name,
+ &syslogFile))
+ return;
- 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;
- }
-
- if (filename)
- pfree(filename);
- if (csvfilename)
- pfree(csvfilename);
- return;
- }
-
- 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;
- }
-
- /*
- * 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);
-
- 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;
- }
-
- if (filename)
- pfree(filename);
- if (csvfilename)
- pfree(csvfilename);
- return;
- }
-
- if (csvlogFile != NULL)
- fclose(csvlogFile);
- csvlogFile = fh;
-
- /* 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;
- }
-
- if (filename)
- pfree(filename);
- if (csvfilename)
- pfree(csvfilename);
+ /* 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 +1497,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(),
--
2.33.0
signature.asc
Description: PGP signature
