public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Sehrope Sarkuni <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: [email protected]
Subject: Re: Add jsonlog log_destination for JSON server logs
Date: Tue, 28 Sep 2021 12:30:10 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAH7T-aqkmrCEpTqvwUOWNATitWCnPPf3M75+af8c4afMEbMepQ@mail.gmail.com>
References: <CAH7T-aoDTnRo-U1Etw35bJiJZow7f2euC0Fh6nhSkSSxz333GA@mail.gmail.com>
<CAH7T-aqW0_yxuqznFkQbjdZz1ck+96Q2HzZHki6dsQbVHxzEPw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAH7T-areunm7CBDMLo8jMJ1fs=87eTqRmKOAY6CoEkexfdxEaQ@mail.gmail.com>
<[email protected]>
<CAH7T-apxdwf7=CHD_Vuo+QgtV6TEnLc0N0_SMTJ-a-jFdOBOvA@mail.gmail.com>
<[email protected]>
<CAH7T-aqkmrCEpTqvwUOWNATitWCnPPf3M75+af8c4afMEbMepQ@mail.gmail.com>
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
Attachments:
[text/x-diff] v7-0001-Refactor-fd-handling-when-forking-syslogger-in-EX.patch (4.2K, ../[email protected]/2-v7-0001-Refactor-fd-handling-when-forking-syslogger-in-EX.patch)
download | inline diff:
From 57bcaf49bd10ec3e6afb14f3ec384cb88a65efe6 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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
[text/x-diff] v7-0002-Refactor-per-destination-file-rotation-in-syslogg.patch (8.9K, ../[email protected]/3-v7-0002-Refactor-per-destination-file-rotation-in-syslogg.patch)
download | inline diff:
From 88d43ad72b252e9efd2c9b7f5ba84d786ad78471 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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
[application/pgp-signature] signature.asc (833B, ../[email protected]/4-signature.asc)
download
view thread (27+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Add jsonlog log_destination for JSON server logs
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox