From 5c6e33f071537d9831db57471a06d39a175b535a Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Thu, 7 Oct 2021 10:12:45 +0900 Subject: [PATCH] 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 --- src/backend/postmaster/syslogger.c | 243 ++++++++++++++--------------- 1 file changed, 120 insertions(+), 123 deletions(-) diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c index c5f9c5202d..34b16ba3bb 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; /* @@ -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"); @@ -1241,16 +1245,115 @@ 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, or 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 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; + } + + /* + * 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 && (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 +1366,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 +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(),